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

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