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

[Devin Heitmueller]

Hi Marton,

Sorry, I'm now recognizing I should have answered this email prior to
the later one.  Comments inline:

On Tue, Apr 25, 2023 at 5:59 PM Marton Balint <cus at passwd.hu> wrote:
> > 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.

I have a subsequent patch which supports multiple audio output streams
(which may be a mix of PCM and compressed audio).  Those streams need
to be interleaved together before submitting them to the hardware.  I
made a fundamental change to the design such that I employ an
intermediate FIFO which contains the interleaved audio, and the
submission to the hardware is done in the audio callback as we get
close to the scheduling deadline (which runs on a separate thread and
thus the queue needs to be thread-safe).

I am quite confident that considerable discussion will be needed to
explain why I arrived at this design decision, as even I will
acknowledge that it seems ugly at first inspection.  The design has
actually evolved three or four times over the last five years as I had
to address a variety of edge cases found in real-world usage and
working in low-latency environments.

> > 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.

Ok.

> 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?

Sure, I can take a look.  I am definitely in favor of using common
functions, and it wasn't until I looked more closely at the code did I
recognize why the author wrote yet another FIFO implementation rather
than using one of the standard public ones.

If we can end up with the decklink queue being a simple wrapper around
avpriv_packet_list() but with an added mutex, then I think that would
be ideal.

> 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.

I never really liked the naming either, and agree that it implies the
functionality is public rather than private to decklink.  I can submit
a patch renaming the functions.

Devin

-- 
Devin Heitmueller, Senior Software Engineer
LTN Global Communications
o: +1 (301) 363-1001
w: https://ltnglobal.com  e: devin.heitmueller at ltnglobal.com