diff mbox series

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

Message ID 1593610798-21093-3-git-send-email-lance.lmwang@gmail.com
State Superseded
Headers show
Series [FFmpeg-devel,v6,1/3] avutil/timecode: add description for SMPTE binary format
Related show

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Limin Wang July 1, 2020, 1:39 p.m. UTC
From: Limin Wang <lance.lmwang@gmail.com>

Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
---
 libavdevice/decklink_dec.cpp | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

Comments

Limin Wang July 8, 2020, 1:13 a.m. UTC | #1
On Wed, Jul 01, 2020 at 09:39:58PM +0800, lance.lmwang@gmail.com wrote:
> From: Limin Wang <lance.lmwang@gmail.com>
> 
> Signed-off-by: Limin Wang <lance.lmwang@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;
> +
> +                        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) {
> +                                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.
Marton Balint July 8, 2020, 9:59 p.m. UTC | #2
On Wed, 8 Jul 2020, lance.lmwang@gmail.com wrote:

> On Wed, Jul 01, 2020 at 09:39:58PM +0800, lance.lmwang@gmail.com wrote:
>> From: Limin Wang <lance.lmwang@gmail.com>
>> 
>> Signed-off-by: Limin Wang <lance.lmwang@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.

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

Also in theory you could query the color framing flag from the decklink 
api.

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@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Limin Wang July 8, 2020, 11:34 p.m. UTC | #3
On Wed, Jul 08, 2020 at 11:59:17PM +0200, Marton Balint wrote:
> 
> 
> On Wed, 8 Jul 2020, lance.lmwang@gmail.com wrote:
> 
> > On Wed, Jul 01, 2020 at 09:39:58PM +0800, lance.lmwang@gmail.com wrote:
> > > From: Limin Wang <lance.lmwang@gmail.com>
> > > 
> > > Signed-off-by: Limin Wang <lance.lmwang@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. Do you insist that
this version must support this feature?

> 
> 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@ffmpeg.org
> > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> > 
> > To unsubscribe, visit link above, or email
> > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Marton Balint July 9, 2020, 9:15 p.m. UTC | #4
On Thu, 9 Jul 2020, lance.lmwang@gmail.com wrote:

> On Wed, Jul 08, 2020 at 11:59:17PM +0200, Marton Balint wrote:
>> 
>> 
>> On Wed, 8 Jul 2020, lance.lmwang@gmail.com wrote:
>> 
>> > On Wed, Jul 01, 2020 at 09:39:58PM +0800, lance.lmwang@gmail.com wrote:
>> > > From: Limin Wang <lance.lmwang@gmail.com>
>> > > 
>> > > Signed-off-by: Limin Wang <lance.lmwang@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?

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@ffmpeg.org
>> > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>> > 
>> > To unsubscribe, visit link above, or email
>> > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>> 
>> To unsubscribe, visit link above, or email
>> ffmpeg-devel-request@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-request@ffmpeg.org with subject "unsubscribe".
Limin Wang July 10, 2020, 1:54 a.m. UTC | #5
On Thu, Jul 09, 2020 at 11:15:51PM +0200, Marton Balint wrote:
> 
> 
> On Thu, 9 Jul 2020, lance.lmwang@gmail.com wrote:
> 
> > On Wed, Jul 08, 2020 at 11:59:17PM +0200, Marton Balint wrote:
> > > 
> > > 
> > > On Wed, 8 Jul 2020, lance.lmwang@gmail.com wrote:
> > > 
> > > > On Wed, Jul 01, 2020 at 09:39:58PM +0800, lance.lmwang@gmail.com wrote:
> > > > > From: Limin Wang <lance.lmwang@gmail.com>
> > > > > > > Signed-off-by: Limin Wang <lance.lmwang@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@ffmpeg.org
> > > > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> > > > > To unsubscribe, visit link above, or email
> > > > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
> > > _______________________________________________
> > > ffmpeg-devel mailing list
> > > ffmpeg-devel@ffmpeg.org
> > > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> > > 
> > > To unsubscribe, visit link above, or email
> > > ffmpeg-devel-request@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-request@ffmpeg.org with subject "unsubscribe".
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
diff mbox series

Patch

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;
+
+                        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) {
+                                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);