[FFmpeg-devel] [PATCH 0/2] Implement SMPTE 2038 output support over Decklink SDI

[Marton Balint]

On Mon, 24 Apr 2023, Devin Heitmueller wrote:

> Hello Marton,
>
> Thanks for reviewing.  Comments inline:
>
> On Sun, Apr 23, 2023 at 2:43 PM Marton Balint <cus at passwd.hu> wrote:

[...]

Thanks for the detailed explanations. I guess then keeping the queue is 
well justified here.

> Regarding the use of avpriv_packet_list() as opposed to
> avpacket_queue_*, I used the avpacket_queue functions for consistency
> with the decklink capture module where it is used today.  Also,
> avpacket_queue is threadsafe while avpriv_packet_list.*() is not.
> While the threadsafeness is not critical for the VANC case, I have
> subsequent patches for audio where it is important, and I figured it
> would more consistent to use the same queue mechanism within decklink
> for all three (capture, audio output, and vanc output).

Can you explain how thread safety will be relevant for audio? The 
muxer should get packets in a thread safe way, so I don't quite see how 
suddenly it will be needed.

>
> That said, I wouldn't specifically object to converting to the
> avpriv_packet_list functions since thread-safeness isn't really a
> requirement for this particular case.  It's probably worth noting
> though that I extended the avpacket_queue method to allow me to peek
> at the first packet in the queue (which avpriv_packet_list doesn't
> support today).  Hence converting to avpriv_packet_list would require
> an equivalent addition to be accepted upstream.

You can access the internals of the PacketList struct, so you can just add 
needed function to your own code, you don't necessarily have to make it 
public. On the other hand, the avpriv_packet_list does not have the 
concept of queue size or queue count, so it is not only thread safety 
that will be missing.

Two things bother me with the decklink queue:

1) It duplicates the functionality of avpriv_packet_list_put and 
avpriv_packet_list_get, but it seems to me it should not be difficult 
to actually use these get/put functions in the decklink queue as well, 
because it is already using the same packet list struct internally.
Maybe can you give it a try?

2) Namespacing of the struct / functions are wrong. Struct is called 
AVPacketQueue, it should be something like DecklinkPacketQueue in order 
to make it clear that it is not a public struct. The function names are 
prefixed with avpacket, which is also wrong. It should be simply 
packet_queue_xxx, av* would imply a public function. And if you 
factorize it to a non-static function, then it should be 
ff_decklink_packet_queue_xxx.

With these two things fixed, things would look a lot better :)

Regards,
Marton