Refactor receiveFrame and sendPacket logic to dispatch directly to interface

[mollyxu]

Removed canDecodePacketDirectly() and refactored decoding loop's receiveFrame and sendPacket logic to dispatch directly to interface

[NicolasHug]

Just pointing for myself and for other reviewers that the call to initialize() was moved. Which makes sense, we now pass the codecContext to initialize(), and the codecContext is only initialized after the call to avcodec_open2.

We should just make sure there was no other call to the interface method that would depend on the interface being initialized. I think this is OK, the only call was deviceInterface_->findCodec(), and it's pretty much a stateless check.

[scotts]

Yeah, I think we're good here. My rationale with the device was to initialize it as soon as we had everything we needed to do it.

[NicolasHug]

It'd be nice if we could have this just be:

streamInfo.codecContext = SharedAVCodecContext(codecContext);

and have the definition of SharedAVCodecContext specify its own destructor. This is what we do for the other smart pointers in FFmpegCommon.h.

(It may not be possible or too clumsy, I gave it a quick try to better illustrate what I mean, and I wasn't able to make it work!)

[mollyxu]

I think the deleter is not a part of type std::shared_ptr so there would need to be a helper function makeSharedAVCodecContext in FFMPEGCommon.h and the usage would be

streamInfo.codecContext = makeSharedAVCodecContext(codecContext);

Would this be preferred?

[scotts]

@mollyxu, take a look at UniqueAVCodecContext, which is defined right above your new SharedAVCodecContext. That makes the deleter part of the using statement, which enables us to just say UniqueAVCodecContext(ptr) in normal code.

[NicolasHug]

I looked at this again because I wanted to figure out why I couldn't make it work yesterday. It seems we can't use the same template parameter syntax as we do for unique_ptr, because shared_ptr only accepts the deleter in the constructor. So we can't define

using SharedAVCodecContext = std::shared_ptr<
   AVCodecContext,
   Deleterp<AVCodecContext, void, avcodec_free_context>>;

which is what I was hoping for. That's just not supported by the shared_ptr type.

The closest I got is to define a construction helper

using SharedAVCodecContext = std::shared_ptr<AVCodecContext>;
inline SharedAVCodecContext makeSharedAVCodecContext(AVCodecContext* ctx) {
  return SharedAVCodecContext(ctx, Deleterp<AVCodecContext, void, avcodec_free_context>{});
}

which is then used as

  streamInfo.codecContext = makeSharedAVCodecContext(codecContext);

(When I say I, I mean Claude).

It's very close to what you already have, the main difference is that we're using our Deleterp template instead of a lambda. I'd still suggest to use makeSharedAVCodecContext, so that all our smart pointer logic is centralized within the same FFmpegCommon.h file.

(If we move makeSharedAVCodecContext to the .cpp file we can remove the inline, but I don't think it's worth it).

[scotts]

@NicolasHug, thanks for digging into this! I am resisting the temptation to dive into the C++ docs to figure out why these two seemingly complementary classes have different interfaces! Not a joke. :)

[NicolasHug]

I was also wondering and got this:

  Why the difference?

  The key difference is that std::unique_ptr needs to know the deleter type at compile time because it affects the
  size of the object. But std::shared_ptr uses type erasure - it stores the deleter internally in a way that
  doesn't affect the shared_ptr type itself.

  This means:
  - std::unique_ptr<int, CustomDeleter1> and std::unique_ptr<int, CustomDeleter2> are different types
  - std::shared_ptr<int> with CustomDeleter1 and std::shared_ptr<int> with CustomDeleter2 are the same type

But this isn't really a great reason. This explain why unique_ptr cannot be passed the deleter to its constructor. But it doesn't explain why shared_ptr doesn't support have the same syntax as unique_ptr. 🤔

[mollyxu]

Thanks for the suggestions and for taking the time to look into this!

[NicolasHug]

I left one suggestion on how to deal with the SharedAVCodecContext definition in #954 (comment). PTAL, I'll approve now because I think it should be easy to address, and I don't want to block this further with another review round.

Thank you for the PR @mollyxu , our main decoding loop is much neater now, and this change will also greatly simplify future improvements like #926 and #943