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 <c...@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
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
