On Sun, Jul 12, 2020 at 03:07:18AM +0200, Andreas Rheinhardt wrote: > lance.lmw...@gmail.com: > > From: Limin Wang <lance.lmw...@gmail.com> > > > > Signed-off-by: Limin Wang <lance.lmw...@gmail.com> > > --- > > libavdevice/decklink_dec.cpp | 14 ++++++++++++++ > > 1 file changed, 14 insertions(+) > > > > diff --git a/libavdevice/decklink_dec.cpp b/libavdevice/decklink_dec.cpp > > index a499972..146f56f 100644 > > --- a/libavdevice/decklink_dec.cpp > > +++ b/libavdevice/decklink_dec.cpp > > @@ -42,6 +42,7 @@ extern "C" { > > #include "libavutil/imgutils.h" > > #include "libavutil/intreadwrite.h" > > #include "libavutil/time.h" > > +#include "libavutil/timecode.h" > > #include "libavutil/mathematics.h" > > #include "libavutil/reverse.h" > > #include "avdevice.h" > > @@ -882,6 +883,19 @@ HRESULT > > decklink_input_callback::VideoInputFrameArrived( > > AVDictionary* metadata_dict = NULL; > > int metadata_len; > > uint8_t* packed_metadata; > > + AVTimecode tcr; > > + > > + if (av_timecode_init_from_string(&tcr, > > ctx->video_st->r_frame_rate, tc, ctx) >= 0) { > > + uint32_t tc_data = > > av_timecode_get_smpte_from_framenum(&tcr, 0); > > + int size = sizeof(uint32_t) * 4; > > + uint8_t *sd = av_packet_new_side_data(&pkt, > > AV_PKT_DATA_S12M_TIMECODE, size); > > + > > + if (sd) { > > + AV_WL32(sd, 1); // one TC ; > > Nit: why is there a space after TC?
Will remove it, it's not intentional. > > > + AV_WL32(sd + 4, tc_data); // TC; > > This contradicts the documentation of AV_PKT_DATA_S12M_TIMECODE, because > you are always using little endian here. But the documentation [1] does > not specify an endianness, it uses native endianness (in accordance with > what AV_FRAME_DATA_S12M_TIMECODE does). This very same patch also uses > native endianness in libavformat/dump.c. good catch, at first, I think it's little endian order, and Nicolas comment uint32_t is native, so I remove the commens for the byte order. I'll change to use AV_WB32() for native write. > > I consider it a mistake to use native endianness for the frame side data > and an uint32_t in av_timecode_get_smpte (none of the fields cross a > byte boundary, so each entry could easily have been an uint8_t[4]), as > this will make it harder to test this side data via fate; but I also see > the advantage of using the same format and the same helper functions for > both. What do others think about this? After all, we can still change > the format for the packet side data. I didn't get the SMPTE S314M:2005 specs for checking, it's not free. I think use native format is fine for local system, but if the side data is packed into or read from specific format like DV, AV_WB32() and AV_RB32() had to be used. For the fate testing, I think we can't test the side data binary only, if needed, we can test the format which use the sidedata like dv. I have add fate timecode testing for h264/hevc and haven't submit yet. But if the frame rate > 30, I got one unexpected result after map SMPTE ST 12-1:2014 side data to HEVC timecode, the frame is 6bit only(2bit for tens of frame), so to framerate > 30, it'll be divided by 2 with same timecode, but HEVC timecode frame number can use 9bit and expect the frame > 30. > > - Andreas > > [1]: > https://patchwork.ffmpeg.org/project/ffmpeg/patch/1593610798-21093-2-git-send-email-lance.lmw...@gmail.com/ > (Is this even the latest version? I haven't paid close attention to this > patchset tbh.) Yes, in my local system, I change the version related for conflict. > _______________________________________________ > 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". -- Thanks, Limin Wang _______________________________________________ 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".