On Sun, Jul 12, 2020 at 06:03:04PM +0200, Andreas Rheinhardt wrote: > lance.lmw...@gmail.com: > > 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. > > > > AV_WB uses big-endian. Instead you could simply use an uint32_t* to > write the numbers.
OK, I'll change it. > > >> > >> 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. > > The framecrc muxer calculates a checksum of every side data. There is > already a special way of calculating the side-data for palettes in > big-endian and it seems that we can simply reuse this (as both are based > on uint32_t in native format). So it seems it won't be much of a problem. > > - Andreas > _______________________________________________ > 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".