[FFmpeg-devel] [PATCH v8 3/3] avdevice/decklink_dec: export timecode with s12m side data

[lance.lmwang at gmail.com]

On Sun, Jul 12, 2020 at 06:03:04PM +0200, Andreas Rheinhardt wrote:
> lance.lmwang at gmail.com:
> > On Sun, Jul 12, 2020 at 03:07:18AM +0200, Andreas Rheinhardt wrote:
> >> lance.lmwang at gmail.com:
> >>> From: Limin Wang <lance.lmwang at gmail.com>
> >>>
> >>> Signed-off-by: Limin Wang <lance.lmwang at 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 at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".

-- 
Thanks,
Limin Wang