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

[lance.lmwang at gmail.com]

On Thu, Jul 09, 2020 at 11:15:51PM +0200, Marton Balint wrote:
> 
> 
> On Thu, 9 Jul 2020, lance.lmwang at gmail.com wrote:
> 
> > On Wed, Jul 08, 2020 at 11:59:17PM +0200, Marton Balint wrote:
> > > 
> > > 
> > > On Wed, 8 Jul 2020, lance.lmwang at gmail.com wrote:
> > > 
> > > > On Wed, Jul 01, 2020 at 09:39:58PM +0800, lance.lmwang at gmail.com wrote:
> > > > > From: Limin Wang <lance.lmwang at gmail.com>
> > > > > > > Signed-off-by: Limin Wang <lance.lmwang at gmail.com>
> > > > > ---
> > > > >  libavdevice/decklink_dec.cpp | 16 ++++++++++++++++
> > > > >  1 file changed, 16 insertions(+)
> > > > > > > diff --git a/libavdevice/decklink_dec.cpp
> > > b/libavdevice/decklink_dec.cpp
> > > > > index 82106aa..0fc1489 100644
> > > > > --- a/libavdevice/decklink_dec.cpp
> > > > > +++ b/libavdevice/decklink_dec.cpp
> > > > > @@ -41,6 +41,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"
> > > > > @@ -778,6 +779,21 @@ HRESULT decklink_input_callback::VideoInputFrameArrived(
> > > > >                          AVDictionary* metadata_dict = NULL;
> > > > >                          int metadata_len;
> > > > >                          uint8_t* packed_metadata;
> > > > > +                        AVTimecode tcr;
> > > > > +                        uint32_t tc_data;
> > > > > +                        uint8_t *sd;
> > > > > +                        int size = sizeof(uint32_t) * 4;
> > > 
> > > You can push some of these initializers into the blocks in which they are
> > > used.
> > 
> > Sure, will fix it.
> > 
> > > 
> > > > > +
> > > > > +                        if (av_timecode_init_from_string(&tcr, ctx->video_st->r_frame_rate, tc, ctx) >= 0) {
> > > > > +                            if ((tc_data = av_timecode_get_smpte_from_framenum(&tcr, 0)) > 0) {
> > > 
> > > av_timecode_get_smpte_from_framenum() always succeeds, so this check is
> > > wrong.
> > 
> > OK, will fix it.
> > 
> > > 
> > > Also in theory you could query the color framing flag from the decklink api.
> > 
> > av_timecode_get_smpte_from_framenum() assume the Color Frame flag is zero
> > always, and the conversion from frame to string didn't support the color frame
> > flag also, so even we set the flag, it's not used still.
> 
> It is a matter of setting a single bit of tc_data, you don't need to add API
> for it.
> 
> > Do you insist that this version must support this feature?
> 
> No, but if the DeckLink API supports it, and the side data supports it then
> why not? It should be no more than 2 lines of additional code:
> 
> unsigned color_flag = !!(timecode->GetFlags() & bmdTimecodeColorFrame);
> 
> ...
> 
> tc_data |= color_flag << 31;
> 
> Or am I missing something?

No, your change is good to me, but I don't know how to create such input test signal
with the flag is on, so I can't test it really. So I prefer to add it in future
with separate patch, I'll update the patch without the color_flag support.

> 
> Regards,
> Marton
> 
> > 
> > > 
> > > Regards,
> > > Marton
> > > 
> > > > > +                                sd = av_packet_new_side_data(&pkt, AV_PKT_DATA_S12M_TIMECODE, size);
> > > > > +                                if (sd) {
> > > > > +                                    AV_WL32(sd, 1); // one TC ;
> > > > > +                                    AV_WL32(sd + 4, tc_data); // TC;
> > > > > +                                }
> > > > > +                            }
> > > > > +                        }
> > > > > +
> > > > >                          if (av_dict_set(&metadata_dict, "timecode", tc, AV_DICT_DONT_STRDUP_VAL) >= 0) {
> > > > >                              packed_metadata = av_packet_pack_dictionary(metadata_dict, &metadata_len);
> > > > >                              av_dict_free(&metadata_dict);
> > > > > -- > > 1.8.3.1
> > > > > > > Marton, please help to review and give comments.
> > > > > -- > Thanks,
> > > > Limin Wang
> > > > _______________________________________________
> > > > 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".
> > > _______________________________________________
> > > 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
> > _______________________________________________
> > 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".
> _______________________________________________
> 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