diff mbox series

[FFmpeg-devel,2/4] API: add AV_PKT_DATA_S12M_TIMECODE to AVPacketSideDataType

Message ID 1592577334-19049-2-git-send-email-lance.lmwang@gmail.com
State New
Headers show
Series [FFmpeg-devel,1/4] avfilter/vf_drawtext: add option to draw timecode in S12M side data
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 June 19, 2020, 2:35 p.m. UTC
From: Limin Wang <lance.lmwang@gmail.com>

Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
---
 doc/APIchanges        |  3 +++
 libavcodec/avpacket.c |  1 +
 libavcodec/decode.c   |  1 +
 libavcodec/packet.h   |  8 ++++++++
 libavcodec/version.h  |  2 +-
 libavformat/dump.c    | 22 ++++++++++++++++++++++
 6 files changed, 36 insertions(+), 1 deletion(-)

Comments

Andreas Rheinhardt June 29, 2020, 2:53 a.m. UTC | #1
lance.lmwang@gmail.com:
> From: Limin Wang <lance.lmwang@gmail.com>
> 
> Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> ---
>  doc/APIchanges        |  3 +++
>  libavcodec/avpacket.c |  1 +
>  libavcodec/decode.c   |  1 +
>  libavcodec/packet.h   |  8 ++++++++
>  libavcodec/version.h  |  2 +-
>  libavformat/dump.c    | 22 ++++++++++++++++++++++
>  6 files changed, 36 insertions(+), 1 deletion(-)
> 
> diff --git a/doc/APIchanges b/doc/APIchanges
> index 1d6cc36..7cad7fa 100644
> --- a/doc/APIchanges
> +++ b/doc/APIchanges
> @@ -15,6 +15,9 @@ libavutil:     2017-10-21
>  
>  API changes, most recent first:
>  
> +2020-06-xx - xxxxxxxxxx - lavc 58.94.100 - packet.h
> +  Add AV_PKT_DATA_S12M_TIMECODE.
> +
>  2020-06-12 - b09fb030c1 - lavu 56.55.100 - pixdesc.h
>    Add AV_PIX_FMT_X2RGB10.
>  
> diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
> index dce26cb..a4316a7 100644
> --- a/libavcodec/avpacket.c
> +++ b/libavcodec/avpacket.c
> @@ -400,6 +400,7 @@ const char *av_packet_side_data_name(enum AVPacketSideDataType type)
>      case AV_PKT_DATA_PRFT:                       return "Producer Reference Time";
>      case AV_PKT_DATA_ICC_PROFILE:                return "ICC Profile";
>      case AV_PKT_DATA_DOVI_CONF:                  return "DOVI configuration record";
> +    case AV_PKT_DATA_S12M_TIMECODE:              return "SMPTE 12-1 timecode";
>      }
>      return NULL;
>  }
> diff --git a/libavcodec/decode.c b/libavcodec/decode.c
> index a4e50c0..a2bf0cc 100644
> --- a/libavcodec/decode.c
> +++ b/libavcodec/decode.c
> @@ -1699,6 +1699,7 @@ int ff_decode_frame_props(AVCodecContext *avctx, AVFrame *frame)
>          { AV_PKT_DATA_CONTENT_LIGHT_LEVEL,        AV_FRAME_DATA_CONTENT_LIGHT_LEVEL },
>          { AV_PKT_DATA_A53_CC,                     AV_FRAME_DATA_A53_CC },
>          { AV_PKT_DATA_ICC_PROFILE,                AV_FRAME_DATA_ICC_PROFILE },
> +        { AV_PKT_DATA_S12M_TIMECODE,              AV_FRAME_DATA_S12M_TIMECODE },
>      };
>  
>      if (pkt) {
> diff --git a/libavcodec/packet.h b/libavcodec/packet.h
> index 41485f4..72cb4f7 100644
> --- a/libavcodec/packet.h
> +++ b/libavcodec/packet.h
> @@ -283,6 +283,14 @@ enum AVPacketSideDataType {
>      AV_PKT_DATA_DOVI_CONF,
>  
>      /**
> +     * Timecode which conforms to SMPTE ST 12-1. The data is an array of 4 uint32_t
> +     * where the first uint32_t describes how many (1-3) of the other timecodes are used.
> +     * The timecode format is described in the av_timecode_get_smpte_from_framenum()
> +     * function in libavutil/timecode.c.

The format is documented in a C source file, not a public header? Sure
about that?

> +     */
> +    AV_PKT_DATA_S12M_TIMECODE,
> +
> +    /**
>       * The number of side data types.
>       * This is not part of the public API/ABI in the sense that it may
>       * change when new side data types are added.
> diff --git a/libavcodec/version.h b/libavcodec/version.h
> index 0359302..482cc6d 100644
> --- a/libavcodec/version.h
> +++ b/libavcodec/version.h
> @@ -28,7 +28,7 @@
>  #include "libavutil/version.h"
>  
>  #define LIBAVCODEC_VERSION_MAJOR  58
> -#define LIBAVCODEC_VERSION_MINOR  93
> +#define LIBAVCODEC_VERSION_MINOR  94
>  #define LIBAVCODEC_VERSION_MICRO 100
>  
>  #define LIBAVCODEC_VERSION_INT  AV_VERSION_INT(LIBAVCODEC_VERSION_MAJOR, \
> diff --git a/libavformat/dump.c b/libavformat/dump.c
> index 117c681..c5f0bd6 100644
> --- a/libavformat/dump.c
> +++ b/libavformat/dump.c
> @@ -34,6 +34,7 @@
>  #include "libavutil/replaygain.h"
>  #include "libavutil/spherical.h"
>  #include "libavutil/stereo3d.h"
> +#include "libavutil/timecode.h"
>  
>  #include "avformat.h"
>  
> @@ -402,6 +403,23 @@ static void dump_dovi_conf(void *ctx, AVPacketSideData* sd)
>             dovi->dv_bl_signal_compatibility_id);
>  }
>  
> +static void dump_s12m_timecode(void *ctx, AVPacketSideData* sd)
> +{
> +    uint32_t *tc = (uint32_t*)sd->data;
> +    int m = tc[0] & 3;
> +
> +    if (sd->size != sizeof(uint32_t) * 4) {
> +        av_log(ctx, AV_LOG_ERROR, "invalid data");
> +        return;
> +    }
> +
> +    for (int j = 1; j <= m; j++) {
> +        char tcbuf[AV_TIMECODE_STR_SIZE];
> +        av_timecode_make_smpte_tc_string(tcbuf, tc[j], 0);
> +        av_log(ctx, AV_LOG_INFO, "timecode - %s%s", tcbuf, j != m ? ", " : "");
> +    }
> +}
> +
>  static void dump_sidedata(void *ctx, AVStream *st, const char *indent)
>  {
>      int i;
> @@ -468,6 +486,10 @@ static void dump_sidedata(void *ctx, AVStream *st, const char *indent)
>              av_log(ctx, AV_LOG_INFO, "DOVI configuration record: ");
>              dump_dovi_conf(ctx, &sd);
>              break;
> +        case AV_PKT_DATA_S12M_TIMECODE:
> +            av_log(ctx, AV_LOG_INFO, "S12M timecode: ");
> +            dump_s12m_timecode(ctx, &sd);
> +            break;
>          default:
>              av_log(ctx, AV_LOG_INFO,
>                     "unknown side data type %d (%d bytes)", sd.type, sd.size);
>
Limin Wang June 29, 2020, 3:20 a.m. UTC | #2
On Mon, Jun 29, 2020 at 04:53:21AM +0200, Andreas Rheinhardt wrote:
> lance.lmwang@gmail.com:
> > From: Limin Wang <lance.lmwang@gmail.com>
> > 
> > Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> > ---
> >  doc/APIchanges        |  3 +++
> >  libavcodec/avpacket.c |  1 +
> >  libavcodec/decode.c   |  1 +
> >  libavcodec/packet.h   |  8 ++++++++
> >  libavcodec/version.h  |  2 +-
> >  libavformat/dump.c    | 22 ++++++++++++++++++++++
> >  6 files changed, 36 insertions(+), 1 deletion(-)
> > 
> > diff --git a/doc/APIchanges b/doc/APIchanges
> > index 1d6cc36..7cad7fa 100644
> > --- a/doc/APIchanges
> > +++ b/doc/APIchanges
> > @@ -15,6 +15,9 @@ libavutil:     2017-10-21
> >  
> >  API changes, most recent first:
> >  
> > +2020-06-xx - xxxxxxxxxx - lavc 58.94.100 - packet.h
> > +  Add AV_PKT_DATA_S12M_TIMECODE.
> > +
> >  2020-06-12 - b09fb030c1 - lavu 56.55.100 - pixdesc.h
> >    Add AV_PIX_FMT_X2RGB10.
> >  
> > diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
> > index dce26cb..a4316a7 100644
> > --- a/libavcodec/avpacket.c
> > +++ b/libavcodec/avpacket.c
> > @@ -400,6 +400,7 @@ const char *av_packet_side_data_name(enum AVPacketSideDataType type)
> >      case AV_PKT_DATA_PRFT:                       return "Producer Reference Time";
> >      case AV_PKT_DATA_ICC_PROFILE:                return "ICC Profile";
> >      case AV_PKT_DATA_DOVI_CONF:                  return "DOVI configuration record";
> > +    case AV_PKT_DATA_S12M_TIMECODE:              return "SMPTE 12-1 timecode";
> >      }
> >      return NULL;
> >  }
> > diff --git a/libavcodec/decode.c b/libavcodec/decode.c
> > index a4e50c0..a2bf0cc 100644
> > --- a/libavcodec/decode.c
> > +++ b/libavcodec/decode.c
> > @@ -1699,6 +1699,7 @@ int ff_decode_frame_props(AVCodecContext *avctx, AVFrame *frame)
> >          { AV_PKT_DATA_CONTENT_LIGHT_LEVEL,        AV_FRAME_DATA_CONTENT_LIGHT_LEVEL },
> >          { AV_PKT_DATA_A53_CC,                     AV_FRAME_DATA_A53_CC },
> >          { AV_PKT_DATA_ICC_PROFILE,                AV_FRAME_DATA_ICC_PROFILE },
> > +        { AV_PKT_DATA_S12M_TIMECODE,              AV_FRAME_DATA_S12M_TIMECODE },
> >      };
> >  
> >      if (pkt) {
> > diff --git a/libavcodec/packet.h b/libavcodec/packet.h
> > index 41485f4..72cb4f7 100644
> > --- a/libavcodec/packet.h
> > +++ b/libavcodec/packet.h
> > @@ -283,6 +283,14 @@ enum AVPacketSideDataType {
> >      AV_PKT_DATA_DOVI_CONF,
> >  
> >      /**
> > +     * Timecode which conforms to SMPTE ST 12-1. The data is an array of 4 uint32_t
> > +     * where the first uint32_t describes how many (1-3) of the other timecodes are used.
> > +     * The timecode format is described in the av_timecode_get_smpte_from_framenum()
> > +     * function in libavutil/timecode.c.
> 
> The format is documented in a C source file, not a public header? Sure
> about that?

For AV_PKT_DATA_S12M_TIMECODE use the same format with AV_FRAME_DATA_S12M_TIMECODE, so I copy the
comments for AV_FRAME_DATA_S12M_TIMECODE, it's not my comments for it. I think the format is described
in "SMPTE ST 12-2:2014" standard document, but it's not free for download.

What's your suggestion to change?

> 
> > +     */
> > +    AV_PKT_DATA_S12M_TIMECODE,
> > +
> > +    /**
> >       * The number of side data types.
> >       * This is not part of the public API/ABI in the sense that it may
> >       * change when new side data types are added.
> > diff --git a/libavcodec/version.h b/libavcodec/version.h
> > index 0359302..482cc6d 100644
> > --- a/libavcodec/version.h
> > +++ b/libavcodec/version.h
> > @@ -28,7 +28,7 @@
> >  #include "libavutil/version.h"
> >  
> >  #define LIBAVCODEC_VERSION_MAJOR  58
> > -#define LIBAVCODEC_VERSION_MINOR  93
> > +#define LIBAVCODEC_VERSION_MINOR  94
> >  #define LIBAVCODEC_VERSION_MICRO 100
> >  
> >  #define LIBAVCODEC_VERSION_INT  AV_VERSION_INT(LIBAVCODEC_VERSION_MAJOR, \
> > diff --git a/libavformat/dump.c b/libavformat/dump.c
> > index 117c681..c5f0bd6 100644
> > --- a/libavformat/dump.c
> > +++ b/libavformat/dump.c
> > @@ -34,6 +34,7 @@
> >  #include "libavutil/replaygain.h"
> >  #include "libavutil/spherical.h"
> >  #include "libavutil/stereo3d.h"
> > +#include "libavutil/timecode.h"
> >  
> >  #include "avformat.h"
> >  
> > @@ -402,6 +403,23 @@ static void dump_dovi_conf(void *ctx, AVPacketSideData* sd)
> >             dovi->dv_bl_signal_compatibility_id);
> >  }
> >  
> > +static void dump_s12m_timecode(void *ctx, AVPacketSideData* sd)
> > +{
> > +    uint32_t *tc = (uint32_t*)sd->data;
> > +    int m = tc[0] & 3;
> > +
> > +    if (sd->size != sizeof(uint32_t) * 4) {
> > +        av_log(ctx, AV_LOG_ERROR, "invalid data");
> > +        return;
> > +    }
> > +
> > +    for (int j = 1; j <= m; j++) {
> > +        char tcbuf[AV_TIMECODE_STR_SIZE];
> > +        av_timecode_make_smpte_tc_string(tcbuf, tc[j], 0);
> > +        av_log(ctx, AV_LOG_INFO, "timecode - %s%s", tcbuf, j != m ? ", " : "");
> > +    }
> > +}
> > +
> >  static void dump_sidedata(void *ctx, AVStream *st, const char *indent)
> >  {
> >      int i;
> > @@ -468,6 +486,10 @@ static void dump_sidedata(void *ctx, AVStream *st, const char *indent)
> >              av_log(ctx, AV_LOG_INFO, "DOVI configuration record: ");
> >              dump_dovi_conf(ctx, &sd);
> >              break;
> > +        case AV_PKT_DATA_S12M_TIMECODE:
> > +            av_log(ctx, AV_LOG_INFO, "S12M timecode: ");
> > +            dump_s12m_timecode(ctx, &sd);
> > +            break;
> >          default:
> >              av_log(ctx, AV_LOG_INFO,
> >                     "unknown side data type %d (%d bytes)", sd.type, sd.size);
> > 
> 
> _______________________________________________
> 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".
Andreas Rheinhardt June 29, 2020, 12:06 p.m. UTC | #3
lance.lmwang@gmail.com:
> On Mon, Jun 29, 2020 at 04:53:21AM +0200, Andreas Rheinhardt wrote:
>> lance.lmwang@gmail.com:
>>> From: Limin Wang <lance.lmwang@gmail.com>
>>>
>>> Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
>>> ---
>>>  doc/APIchanges        |  3 +++
>>>  libavcodec/avpacket.c |  1 +
>>>  libavcodec/decode.c   |  1 +
>>>  libavcodec/packet.h   |  8 ++++++++
>>>  libavcodec/version.h  |  2 +-
>>>  libavformat/dump.c    | 22 ++++++++++++++++++++++
>>>  6 files changed, 36 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/doc/APIchanges b/doc/APIchanges
>>> index 1d6cc36..7cad7fa 100644
>>> --- a/doc/APIchanges
>>> +++ b/doc/APIchanges
>>> @@ -15,6 +15,9 @@ libavutil:     2017-10-21
>>>  
>>>  API changes, most recent first:
>>>  
>>> +2020-06-xx - xxxxxxxxxx - lavc 58.94.100 - packet.h
>>> +  Add AV_PKT_DATA_S12M_TIMECODE.
>>> +
>>>  2020-06-12 - b09fb030c1 - lavu 56.55.100 - pixdesc.h
>>>    Add AV_PIX_FMT_X2RGB10.
>>>  
>>> diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
>>> index dce26cb..a4316a7 100644
>>> --- a/libavcodec/avpacket.c
>>> +++ b/libavcodec/avpacket.c
>>> @@ -400,6 +400,7 @@ const char *av_packet_side_data_name(enum AVPacketSideDataType type)
>>>      case AV_PKT_DATA_PRFT:                       return "Producer Reference Time";
>>>      case AV_PKT_DATA_ICC_PROFILE:                return "ICC Profile";
>>>      case AV_PKT_DATA_DOVI_CONF:                  return "DOVI configuration record";
>>> +    case AV_PKT_DATA_S12M_TIMECODE:              return "SMPTE 12-1 timecode";
>>>      }
>>>      return NULL;
>>>  }
>>> diff --git a/libavcodec/decode.c b/libavcodec/decode.c
>>> index a4e50c0..a2bf0cc 100644
>>> --- a/libavcodec/decode.c
>>> +++ b/libavcodec/decode.c
>>> @@ -1699,6 +1699,7 @@ int ff_decode_frame_props(AVCodecContext *avctx, AVFrame *frame)
>>>          { AV_PKT_DATA_CONTENT_LIGHT_LEVEL,        AV_FRAME_DATA_CONTENT_LIGHT_LEVEL },
>>>          { AV_PKT_DATA_A53_CC,                     AV_FRAME_DATA_A53_CC },
>>>          { AV_PKT_DATA_ICC_PROFILE,                AV_FRAME_DATA_ICC_PROFILE },
>>> +        { AV_PKT_DATA_S12M_TIMECODE,              AV_FRAME_DATA_S12M_TIMECODE },
>>>      };
>>>  
>>>      if (pkt) {
>>> diff --git a/libavcodec/packet.h b/libavcodec/packet.h
>>> index 41485f4..72cb4f7 100644
>>> --- a/libavcodec/packet.h
>>> +++ b/libavcodec/packet.h
>>> @@ -283,6 +283,14 @@ enum AVPacketSideDataType {
>>>      AV_PKT_DATA_DOVI_CONF,
>>>  
>>>      /**
>>> +     * Timecode which conforms to SMPTE ST 12-1. The data is an array of 4 uint32_t
>>> +     * where the first uint32_t describes how many (1-3) of the other timecodes are used.
>>> +     * The timecode format is described in the av_timecode_get_smpte_from_framenum()
>>> +     * function in libavutil/timecode.c.
>>
>> The format is documented in a C source file, not a public header? Sure
>> about that?
> 
> For AV_PKT_DATA_S12M_TIMECODE use the same format with AV_FRAME_DATA_S12M_TIMECODE, so I copy the
> comments for AV_FRAME_DATA_S12M_TIMECODE, it's not my comments for it. I think the format is described
> in "SMPTE ST 12-2:2014" standard document, but it's not free for download.
> 

It's SMPTE ST 12-1:2014 and this standard is currently (until 21 July)
available for free [1] due to Corona.

> What's your suggestion to change?
> 

The best is probably to incorporate the documentation of the format into
the documentation of av_timecode_get_smpte_from_framenum.

>>
>>> +     */
>>> +    AV_PKT_DATA_S12M_TIMECODE,
>>> +
>>> +    /**
>>>       * The number of side data types.
>>>       * This is not part of the public API/ABI in the sense that it may
>>>       * change when new side data types are added.
>>> diff --git a/libavcodec/version.h b/libavcodec/version.h
>>> index 0359302..482cc6d 100644
>>> --- a/libavcodec/version.h
>>> +++ b/libavcodec/version.h
>>> @@ -28,7 +28,7 @@
>>>  #include "libavutil/version.h"
>>>  
>>>  #define LIBAVCODEC_VERSION_MAJOR  58
>>> -#define LIBAVCODEC_VERSION_MINOR  93
>>> +#define LIBAVCODEC_VERSION_MINOR  94
>>>  #define LIBAVCODEC_VERSION_MICRO 100
>>>  
>>>  #define LIBAVCODEC_VERSION_INT  AV_VERSION_INT(LIBAVCODEC_VERSION_MAJOR, \
>>> diff --git a/libavformat/dump.c b/libavformat/dump.c
>>> index 117c681..c5f0bd6 100644
>>> --- a/libavformat/dump.c
>>> +++ b/libavformat/dump.c
>>> @@ -34,6 +34,7 @@
>>>  #include "libavutil/replaygain.h"
>>>  #include "libavutil/spherical.h"
>>>  #include "libavutil/stereo3d.h"
>>> +#include "libavutil/timecode.h"
>>>  
>>>  #include "avformat.h"
>>>  
>>> @@ -402,6 +403,23 @@ static void dump_dovi_conf(void *ctx, AVPacketSideData* sd)
>>>             dovi->dv_bl_signal_compatibility_id);
>>>  }
>>>  
>>> +static void dump_s12m_timecode(void *ctx, AVPacketSideData* sd)
>>> +{
>>> +    uint32_t *tc = (uint32_t*)sd->data;
>>> +    int m = tc[0] & 3;
>>> +
>>> +    if (sd->size != sizeof(uint32_t) * 4) {

You already dereferenced tc before this check which might crash if the
size is actually < sizeof(uint32_t). And why do you only look at the
lowest two bits of m? Is this designed with future extensions of the
timecode in mind? But even then one can't simply use the upper 30 bits
to store something else, because that has not been documented.
(Moreover, if one allowed extensions, one should allow sd->size to be >=
sizeof(uint32_t) * 4.)

>>> +        av_log(ctx, AV_LOG_ERROR, "invalid data");
>>> +        return;
>>> +    }
>>> +
>>> +    for (int j = 1; j <= m; j++) {
>>> +        char tcbuf[AV_TIMECODE_STR_SIZE];
>>> +        av_timecode_make_smpte_tc_string(tcbuf, tc[j], 0);
>>> +        av_log(ctx, AV_LOG_INFO, "timecode - %s%s", tcbuf, j != m ? ", " : "");
>>> +    }
>>> +}
>>> +

- Andreas

[1]: https://www.smpte.org/free-standards-and-publications
Limin Wang June 29, 2020, 1:37 p.m. UTC | #4
On Mon, Jun 29, 2020 at 02:06:49PM +0200, Andreas Rheinhardt wrote:
> lance.lmwang@gmail.com:
> > On Mon, Jun 29, 2020 at 04:53:21AM +0200, Andreas Rheinhardt wrote:
> >> lance.lmwang@gmail.com:
> >>> From: Limin Wang <lance.lmwang@gmail.com>
> >>>
> >>> Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> >>> ---
> >>>  doc/APIchanges        |  3 +++
> >>>  libavcodec/avpacket.c |  1 +
> >>>  libavcodec/decode.c   |  1 +
> >>>  libavcodec/packet.h   |  8 ++++++++
> >>>  libavcodec/version.h  |  2 +-
> >>>  libavformat/dump.c    | 22 ++++++++++++++++++++++
> >>>  6 files changed, 36 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/doc/APIchanges b/doc/APIchanges
> >>> index 1d6cc36..7cad7fa 100644
> >>> --- a/doc/APIchanges
> >>> +++ b/doc/APIchanges
> >>> @@ -15,6 +15,9 @@ libavutil:     2017-10-21
> >>>  
> >>>  API changes, most recent first:
> >>>  
> >>> +2020-06-xx - xxxxxxxxxx - lavc 58.94.100 - packet.h
> >>> +  Add AV_PKT_DATA_S12M_TIMECODE.
> >>> +
> >>>  2020-06-12 - b09fb030c1 - lavu 56.55.100 - pixdesc.h
> >>>    Add AV_PIX_FMT_X2RGB10.
> >>>  
> >>> diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
> >>> index dce26cb..a4316a7 100644
> >>> --- a/libavcodec/avpacket.c
> >>> +++ b/libavcodec/avpacket.c
> >>> @@ -400,6 +400,7 @@ const char *av_packet_side_data_name(enum AVPacketSideDataType type)
> >>>      case AV_PKT_DATA_PRFT:                       return "Producer Reference Time";
> >>>      case AV_PKT_DATA_ICC_PROFILE:                return "ICC Profile";
> >>>      case AV_PKT_DATA_DOVI_CONF:                  return "DOVI configuration record";
> >>> +    case AV_PKT_DATA_S12M_TIMECODE:              return "SMPTE 12-1 timecode";
> >>>      }
> >>>      return NULL;
> >>>  }
> >>> diff --git a/libavcodec/decode.c b/libavcodec/decode.c
> >>> index a4e50c0..a2bf0cc 100644
> >>> --- a/libavcodec/decode.c
> >>> +++ b/libavcodec/decode.c
> >>> @@ -1699,6 +1699,7 @@ int ff_decode_frame_props(AVCodecContext *avctx, AVFrame *frame)
> >>>          { AV_PKT_DATA_CONTENT_LIGHT_LEVEL,        AV_FRAME_DATA_CONTENT_LIGHT_LEVEL },
> >>>          { AV_PKT_DATA_A53_CC,                     AV_FRAME_DATA_A53_CC },
> >>>          { AV_PKT_DATA_ICC_PROFILE,                AV_FRAME_DATA_ICC_PROFILE },
> >>> +        { AV_PKT_DATA_S12M_TIMECODE,              AV_FRAME_DATA_S12M_TIMECODE },
> >>>      };
> >>>  
> >>>      if (pkt) {
> >>> diff --git a/libavcodec/packet.h b/libavcodec/packet.h
> >>> index 41485f4..72cb4f7 100644
> >>> --- a/libavcodec/packet.h
> >>> +++ b/libavcodec/packet.h
> >>> @@ -283,6 +283,14 @@ enum AVPacketSideDataType {
> >>>      AV_PKT_DATA_DOVI_CONF,
> >>>  
> >>>      /**
> >>> +     * Timecode which conforms to SMPTE ST 12-1. The data is an array of 4 uint32_t
> >>> +     * where the first uint32_t describes how many (1-3) of the other timecodes are used.
> >>> +     * The timecode format is described in the av_timecode_get_smpte_from_framenum()
> >>> +     * function in libavutil/timecode.c.
> >>
> >> The format is documented in a C source file, not a public header? Sure
> >> about that?
> > 
> > For AV_PKT_DATA_S12M_TIMECODE use the same format with AV_FRAME_DATA_S12M_TIMECODE, so I copy the
> > comments for AV_FRAME_DATA_S12M_TIMECODE, it's not my comments for it. I think the format is described
> > in "SMPTE ST 12-2:2014" standard document, but it's not free for download.
> > 
> 
> It's SMPTE ST 12-1:2014 and this standard is currently (until 21 July)
> available for free [1] due to Corona.

oh, it's good to hear it's free due to Corona. I haven't notice it's online.

> 
> > What's your suggestion to change?
> > 
> 
> The best is probably to incorporate the documentation of the format into
> the documentation of av_timecode_get_smpte_from_framenum.
OK, I'll remove the comments for the timecode format.
> 
> >>
> >>> +     */
> >>> +    AV_PKT_DATA_S12M_TIMECODE,
> >>> +
> >>> +    /**
> >>>       * The number of side data types.
> >>>       * This is not part of the public API/ABI in the sense that it may
> >>>       * change when new side data types are added.
> >>> diff --git a/libavcodec/version.h b/libavcodec/version.h
> >>> index 0359302..482cc6d 100644
> >>> --- a/libavcodec/version.h
> >>> +++ b/libavcodec/version.h
> >>> @@ -28,7 +28,7 @@
> >>>  #include "libavutil/version.h"
> >>>  
> >>>  #define LIBAVCODEC_VERSION_MAJOR  58
> >>> -#define LIBAVCODEC_VERSION_MINOR  93
> >>> +#define LIBAVCODEC_VERSION_MINOR  94
> >>>  #define LIBAVCODEC_VERSION_MICRO 100
> >>>  
> >>>  #define LIBAVCODEC_VERSION_INT  AV_VERSION_INT(LIBAVCODEC_VERSION_MAJOR, \
> >>> diff --git a/libavformat/dump.c b/libavformat/dump.c
> >>> index 117c681..c5f0bd6 100644
> >>> --- a/libavformat/dump.c
> >>> +++ b/libavformat/dump.c
> >>> @@ -34,6 +34,7 @@
> >>>  #include "libavutil/replaygain.h"
> >>>  #include "libavutil/spherical.h"
> >>>  #include "libavutil/stereo3d.h"
> >>> +#include "libavutil/timecode.h"
> >>>  
> >>>  #include "avformat.h"
> >>>  
> >>> @@ -402,6 +403,23 @@ static void dump_dovi_conf(void *ctx, AVPacketSideData* sd)
> >>>             dovi->dv_bl_signal_compatibility_id);
> >>>  }
> >>>  
> >>> +static void dump_s12m_timecode(void *ctx, AVPacketSideData* sd)
> >>> +{
> >>> +    uint32_t *tc = (uint32_t*)sd->data;
> >>> +    int m = tc[0] & 3;
> >>> +
> >>> +    if (sd->size != sizeof(uint32_t) * 4) {
> 
> You already dereferenced tc before this check which might crash if the
> size is actually < sizeof(uint32_t). And why do you only look at the
> lowest two bits of m? Is this designed with future extensions of the
> timecode in mind? But even then one can't simply use the upper 30 bits
> to store something else, because that has not been documented.
> (Moreover, if one allowed extensions, one should allow sd->size to be >=
> sizeof(uint32_t) * 4.)

It's from the comments for the side data, it used to avoid index overflow,
for the array is 4. I'll fix to get the m after the checking.

comments:
where the first uint32_t describes how many (1-3) of the other timecodes are used

I think it's for the timecode SEI define 2bits for the count of used timecomes,
it's not related to SMPTE ST 12-1. vf_showinfo.c have dump AV_FRAME_DATA_S12M_TIMECODE,
I'm following it as format is same, but change m to & 3 for I think it's more simple.


> 
> >>> +        av_log(ctx, AV_LOG_ERROR, "invalid data");
> >>> +        return;
> >>> +    }
> >>> +
> >>> +    for (int j = 1; j <= m; j++) {
> >>> +        char tcbuf[AV_TIMECODE_STR_SIZE];
> >>> +        av_timecode_make_smpte_tc_string(tcbuf, tc[j], 0);
> >>> +        av_log(ctx, AV_LOG_INFO, "timecode - %s%s", tcbuf, j != m ? ", " : "");
> >>> +    }
> >>> +}
> >>> +
> 
> - Andreas
> 
> [1]: https://www.smpte.org/free-standards-and-publications
> _______________________________________________
> 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".
Andreas Rheinhardt June 29, 2020, 1:52 p.m. UTC | #5
lance.lmwang@gmail.com:
> On Mon, Jun 29, 2020 at 02:06:49PM +0200, Andreas Rheinhardt wrote:
>> lance.lmwang@gmail.com:
>>> On Mon, Jun 29, 2020 at 04:53:21AM +0200, Andreas Rheinhardt wrote:
>>>> lance.lmwang@gmail.com:
>>>>> From: Limin Wang <lance.lmwang@gmail.com>
>>>>>
>>>>> Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
>>>>> ---
>>>>>  doc/APIchanges        |  3 +++
>>>>>  libavcodec/avpacket.c |  1 +
>>>>>  libavcodec/decode.c   |  1 +
>>>>>  libavcodec/packet.h   |  8 ++++++++
>>>>>  libavcodec/version.h  |  2 +-
>>>>>  libavformat/dump.c    | 22 ++++++++++++++++++++++
>>>>>  6 files changed, 36 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/doc/APIchanges b/doc/APIchanges
>>>>> index 1d6cc36..7cad7fa 100644
>>>>> --- a/doc/APIchanges
>>>>> +++ b/doc/APIchanges
>>>>> @@ -15,6 +15,9 @@ libavutil:     2017-10-21
>>>>>  
>>>>>  API changes, most recent first:
>>>>>  
>>>>> +2020-06-xx - xxxxxxxxxx - lavc 58.94.100 - packet.h
>>>>> +  Add AV_PKT_DATA_S12M_TIMECODE.
>>>>> +
>>>>>  2020-06-12 - b09fb030c1 - lavu 56.55.100 - pixdesc.h
>>>>>    Add AV_PIX_FMT_X2RGB10.
>>>>>  
>>>>> diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
>>>>> index dce26cb..a4316a7 100644
>>>>> --- a/libavcodec/avpacket.c
>>>>> +++ b/libavcodec/avpacket.c
>>>>> @@ -400,6 +400,7 @@ const char *av_packet_side_data_name(enum AVPacketSideDataType type)
>>>>>      case AV_PKT_DATA_PRFT:                       return "Producer Reference Time";
>>>>>      case AV_PKT_DATA_ICC_PROFILE:                return "ICC Profile";
>>>>>      case AV_PKT_DATA_DOVI_CONF:                  return "DOVI configuration record";
>>>>> +    case AV_PKT_DATA_S12M_TIMECODE:              return "SMPTE 12-1 timecode";
>>>>>      }
>>>>>      return NULL;
>>>>>  }
>>>>> diff --git a/libavcodec/decode.c b/libavcodec/decode.c
>>>>> index a4e50c0..a2bf0cc 100644
>>>>> --- a/libavcodec/decode.c
>>>>> +++ b/libavcodec/decode.c
>>>>> @@ -1699,6 +1699,7 @@ int ff_decode_frame_props(AVCodecContext *avctx, AVFrame *frame)
>>>>>          { AV_PKT_DATA_CONTENT_LIGHT_LEVEL,        AV_FRAME_DATA_CONTENT_LIGHT_LEVEL },
>>>>>          { AV_PKT_DATA_A53_CC,                     AV_FRAME_DATA_A53_CC },
>>>>>          { AV_PKT_DATA_ICC_PROFILE,                AV_FRAME_DATA_ICC_PROFILE },
>>>>> +        { AV_PKT_DATA_S12M_TIMECODE,              AV_FRAME_DATA_S12M_TIMECODE },
>>>>>      };
>>>>>  
>>>>>      if (pkt) {
>>>>> diff --git a/libavcodec/packet.h b/libavcodec/packet.h
>>>>> index 41485f4..72cb4f7 100644
>>>>> --- a/libavcodec/packet.h
>>>>> +++ b/libavcodec/packet.h
>>>>> @@ -283,6 +283,14 @@ enum AVPacketSideDataType {
>>>>>      AV_PKT_DATA_DOVI_CONF,
>>>>>  
>>>>>      /**
>>>>> +     * Timecode which conforms to SMPTE ST 12-1. The data is an array of 4 uint32_t
>>>>> +     * where the first uint32_t describes how many (1-3) of the other timecodes are used.
>>>>> +     * The timecode format is described in the av_timecode_get_smpte_from_framenum()
>>>>> +     * function in libavutil/timecode.c.
>>>>
>>>> The format is documented in a C source file, not a public header? Sure
>>>> about that?
>>>
>>> For AV_PKT_DATA_S12M_TIMECODE use the same format with AV_FRAME_DATA_S12M_TIMECODE, so I copy the
>>> comments for AV_FRAME_DATA_S12M_TIMECODE, it's not my comments for it. I think the format is described
>>> in "SMPTE ST 12-2:2014" standard document, but it's not free for download.
>>>
>>
>> It's SMPTE ST 12-1:2014 and this standard is currently (until 21 July)
>> available for free [1] due to Corona.
> 
> oh, it's good to hear it's free due to Corona. I haven't notice it's online.
> 
>>
>>> What's your suggestion to change?
>>>
>>
>> The best is probably to incorporate the documentation of the format into
>> the documentation of av_timecode_get_smpte_from_framenum.
> OK, I'll remove the comments for the timecode format.
>>
>>>>
>>>>> +     */
>>>>> +    AV_PKT_DATA_S12M_TIMECODE,
>>>>> +
>>>>> +    /**
>>>>>       * The number of side data types.
>>>>>       * This is not part of the public API/ABI in the sense that it may
>>>>>       * change when new side data types are added.
>>>>> diff --git a/libavcodec/version.h b/libavcodec/version.h
>>>>> index 0359302..482cc6d 100644
>>>>> --- a/libavcodec/version.h
>>>>> +++ b/libavcodec/version.h
>>>>> @@ -28,7 +28,7 @@
>>>>>  #include "libavutil/version.h"
>>>>>  
>>>>>  #define LIBAVCODEC_VERSION_MAJOR  58
>>>>> -#define LIBAVCODEC_VERSION_MINOR  93
>>>>> +#define LIBAVCODEC_VERSION_MINOR  94
>>>>>  #define LIBAVCODEC_VERSION_MICRO 100
>>>>>  
>>>>>  #define LIBAVCODEC_VERSION_INT  AV_VERSION_INT(LIBAVCODEC_VERSION_MAJOR, \
>>>>> diff --git a/libavformat/dump.c b/libavformat/dump.c
>>>>> index 117c681..c5f0bd6 100644
>>>>> --- a/libavformat/dump.c
>>>>> +++ b/libavformat/dump.c
>>>>> @@ -34,6 +34,7 @@
>>>>>  #include "libavutil/replaygain.h"
>>>>>  #include "libavutil/spherical.h"
>>>>>  #include "libavutil/stereo3d.h"
>>>>> +#include "libavutil/timecode.h"
>>>>>  
>>>>>  #include "avformat.h"
>>>>>  
>>>>> @@ -402,6 +403,23 @@ static void dump_dovi_conf(void *ctx, AVPacketSideData* sd)
>>>>>             dovi->dv_bl_signal_compatibility_id);
>>>>>  }
>>>>>  
>>>>> +static void dump_s12m_timecode(void *ctx, AVPacketSideData* sd)
>>>>> +{
>>>>> +    uint32_t *tc = (uint32_t*)sd->data;
>>>>> +    int m = tc[0] & 3;
>>>>> +
>>>>> +    if (sd->size != sizeof(uint32_t) * 4) {
>>
>> You already dereferenced tc before this check which might crash if the
>> size is actually < sizeof(uint32_t). And why do you only look at the
>> lowest two bits of m? Is this designed with future extensions of the
>> timecode in mind? But even then one can't simply use the upper 30 bits
>> to store something else, because that has not been documented.
>> (Moreover, if one allowed extensions, one should allow sd->size to be >=
>> sizeof(uint32_t) * 4.)
> 
> It's from the comments for the side data, it used to avoid index overflow,
> for the array is 4. I'll fix to get the m after the checking.

I know it is to ensure that you don't overread. I'd simply consider it
invalid data if the count were > 3.

> 
> comments:
> where the first uint32_t describes how many (1-3) of the other timecodes are used
> 
> I think it's for the timecode SEI define 2bits for the count of used timecomes,
> it's not related to SMPTE ST 12-1. vf_showinfo.c have dump AV_FRAME_DATA_S12M_TIMECODE,
> I'm following it as format is same, but change m to & 3 for I think it's more simple.
> 
> 
>>
>>>>> +        av_log(ctx, AV_LOG_ERROR, "invalid data");
>>>>> +        return;
>>>>> +    }
>>>>> +
>>>>> +    for (int j = 1; j <= m; j++) {
>>>>> +        char tcbuf[AV_TIMECODE_STR_SIZE];
>>>>> +        av_timecode_make_smpte_tc_string(tcbuf, tc[j], 0);
>>>>> +        av_log(ctx, AV_LOG_INFO, "timecode - %s%s", tcbuf, j != m ? ", " : "");
>>>>> +    }
>>>>> +}
>>>>> +
>>
>> - Andreas
>>
>> [1]: https://www.smpte.org/free-standards-and-publications
>> _______________________________________________
>> 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 June 29, 2020, 2 p.m. UTC | #6
On Mon, Jun 29, 2020 at 03:52:00PM +0200, Andreas Rheinhardt wrote:
> lance.lmwang@gmail.com:
> > On Mon, Jun 29, 2020 at 02:06:49PM +0200, Andreas Rheinhardt wrote:
> >> lance.lmwang@gmail.com:
> >>> On Mon, Jun 29, 2020 at 04:53:21AM +0200, Andreas Rheinhardt wrote:
> >>>> lance.lmwang@gmail.com:
> >>>>> From: Limin Wang <lance.lmwang@gmail.com>
> >>>>>
> >>>>> Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> >>>>> ---
> >>>>>  doc/APIchanges        |  3 +++
> >>>>>  libavcodec/avpacket.c |  1 +
> >>>>>  libavcodec/decode.c   |  1 +
> >>>>>  libavcodec/packet.h   |  8 ++++++++
> >>>>>  libavcodec/version.h  |  2 +-
> >>>>>  libavformat/dump.c    | 22 ++++++++++++++++++++++
> >>>>>  6 files changed, 36 insertions(+), 1 deletion(-)
> >>>>>
> >>>>> diff --git a/doc/APIchanges b/doc/APIchanges
> >>>>> index 1d6cc36..7cad7fa 100644
> >>>>> --- a/doc/APIchanges
> >>>>> +++ b/doc/APIchanges
> >>>>> @@ -15,6 +15,9 @@ libavutil:     2017-10-21
> >>>>>  
> >>>>>  API changes, most recent first:
> >>>>>  
> >>>>> +2020-06-xx - xxxxxxxxxx - lavc 58.94.100 - packet.h
> >>>>> +  Add AV_PKT_DATA_S12M_TIMECODE.
> >>>>> +
> >>>>>  2020-06-12 - b09fb030c1 - lavu 56.55.100 - pixdesc.h
> >>>>>    Add AV_PIX_FMT_X2RGB10.
> >>>>>  
> >>>>> diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
> >>>>> index dce26cb..a4316a7 100644
> >>>>> --- a/libavcodec/avpacket.c
> >>>>> +++ b/libavcodec/avpacket.c
> >>>>> @@ -400,6 +400,7 @@ const char *av_packet_side_data_name(enum AVPacketSideDataType type)
> >>>>>      case AV_PKT_DATA_PRFT:                       return "Producer Reference Time";
> >>>>>      case AV_PKT_DATA_ICC_PROFILE:                return "ICC Profile";
> >>>>>      case AV_PKT_DATA_DOVI_CONF:                  return "DOVI configuration record";
> >>>>> +    case AV_PKT_DATA_S12M_TIMECODE:              return "SMPTE 12-1 timecode";
> >>>>>      }
> >>>>>      return NULL;
> >>>>>  }
> >>>>> diff --git a/libavcodec/decode.c b/libavcodec/decode.c
> >>>>> index a4e50c0..a2bf0cc 100644
> >>>>> --- a/libavcodec/decode.c
> >>>>> +++ b/libavcodec/decode.c
> >>>>> @@ -1699,6 +1699,7 @@ int ff_decode_frame_props(AVCodecContext *avctx, AVFrame *frame)
> >>>>>          { AV_PKT_DATA_CONTENT_LIGHT_LEVEL,        AV_FRAME_DATA_CONTENT_LIGHT_LEVEL },
> >>>>>          { AV_PKT_DATA_A53_CC,                     AV_FRAME_DATA_A53_CC },
> >>>>>          { AV_PKT_DATA_ICC_PROFILE,                AV_FRAME_DATA_ICC_PROFILE },
> >>>>> +        { AV_PKT_DATA_S12M_TIMECODE,              AV_FRAME_DATA_S12M_TIMECODE },
> >>>>>      };
> >>>>>  
> >>>>>      if (pkt) {
> >>>>> diff --git a/libavcodec/packet.h b/libavcodec/packet.h
> >>>>> index 41485f4..72cb4f7 100644
> >>>>> --- a/libavcodec/packet.h
> >>>>> +++ b/libavcodec/packet.h
> >>>>> @@ -283,6 +283,14 @@ enum AVPacketSideDataType {
> >>>>>      AV_PKT_DATA_DOVI_CONF,
> >>>>>  
> >>>>>      /**
> >>>>> +     * Timecode which conforms to SMPTE ST 12-1. The data is an array of 4 uint32_t
> >>>>> +     * where the first uint32_t describes how many (1-3) of the other timecodes are used.
> >>>>> +     * The timecode format is described in the av_timecode_get_smpte_from_framenum()
> >>>>> +     * function in libavutil/timecode.c.
> >>>>
> >>>> The format is documented in a C source file, not a public header? Sure
> >>>> about that?
> >>>
> >>> For AV_PKT_DATA_S12M_TIMECODE use the same format with AV_FRAME_DATA_S12M_TIMECODE, so I copy the
> >>> comments for AV_FRAME_DATA_S12M_TIMECODE, it's not my comments for it. I think the format is described
> >>> in "SMPTE ST 12-2:2014" standard document, but it's not free for download.
> >>>
> >>
> >> It's SMPTE ST 12-1:2014 and this standard is currently (until 21 July)
> >> available for free [1] due to Corona.
> > 
> > oh, it's good to hear it's free due to Corona. I haven't notice it's online.
> > 
> >>
> >>> What's your suggestion to change?
> >>>
> >>
> >> The best is probably to incorporate the documentation of the format into
> >> the documentation of av_timecode_get_smpte_from_framenum.
> > OK, I'll remove the comments for the timecode format.
> >>
> >>>>
> >>>>> +     */
> >>>>> +    AV_PKT_DATA_S12M_TIMECODE,
> >>>>> +
> >>>>> +    /**
> >>>>>       * The number of side data types.
> >>>>>       * This is not part of the public API/ABI in the sense that it may
> >>>>>       * change when new side data types are added.
> >>>>> diff --git a/libavcodec/version.h b/libavcodec/version.h
> >>>>> index 0359302..482cc6d 100644
> >>>>> --- a/libavcodec/version.h
> >>>>> +++ b/libavcodec/version.h
> >>>>> @@ -28,7 +28,7 @@
> >>>>>  #include "libavutil/version.h"
> >>>>>  
> >>>>>  #define LIBAVCODEC_VERSION_MAJOR  58
> >>>>> -#define LIBAVCODEC_VERSION_MINOR  93
> >>>>> +#define LIBAVCODEC_VERSION_MINOR  94
> >>>>>  #define LIBAVCODEC_VERSION_MICRO 100
> >>>>>  
> >>>>>  #define LIBAVCODEC_VERSION_INT  AV_VERSION_INT(LIBAVCODEC_VERSION_MAJOR, \
> >>>>> diff --git a/libavformat/dump.c b/libavformat/dump.c
> >>>>> index 117c681..c5f0bd6 100644
> >>>>> --- a/libavformat/dump.c
> >>>>> +++ b/libavformat/dump.c
> >>>>> @@ -34,6 +34,7 @@
> >>>>>  #include "libavutil/replaygain.h"
> >>>>>  #include "libavutil/spherical.h"
> >>>>>  #include "libavutil/stereo3d.h"
> >>>>> +#include "libavutil/timecode.h"
> >>>>>  
> >>>>>  #include "avformat.h"
> >>>>>  
> >>>>> @@ -402,6 +403,23 @@ static void dump_dovi_conf(void *ctx, AVPacketSideData* sd)
> >>>>>             dovi->dv_bl_signal_compatibility_id);
> >>>>>  }
> >>>>>  
> >>>>> +static void dump_s12m_timecode(void *ctx, AVPacketSideData* sd)
> >>>>> +{
> >>>>> +    uint32_t *tc = (uint32_t*)sd->data;
> >>>>> +    int m = tc[0] & 3;
> >>>>> +
> >>>>> +    if (sd->size != sizeof(uint32_t) * 4) {
> >>
> >> You already dereferenced tc before this check which might crash if the
> >> size is actually < sizeof(uint32_t). And why do you only look at the
> >> lowest two bits of m? Is this designed with future extensions of the
> >> timecode in mind? But even then one can't simply use the upper 30 bits
> >> to store something else, because that has not been documented.
> >> (Moreover, if one allowed extensions, one should allow sd->size to be >=
> >> sizeof(uint32_t) * 4.)
> > 
> > It's from the comments for the side data, it used to avoid index overflow,
> > for the array is 4. I'll fix to get the m after the checking.
> 
> I know it is to ensure that you don't overread. I'd simply consider it
> invalid data if the count were > 3.

I agree, I'll update to report invalid data if count > 3. To keep consistent,
I'll change the vf_showinfo.c dump to keep the code consistent later.

> 
> > 
> > comments:
> > where the first uint32_t describes how many (1-3) of the other timecodes are used
> > 
> > I think it's for the timecode SEI define 2bits for the count of used timecomes,
> > it's not related to SMPTE ST 12-1. vf_showinfo.c have dump AV_FRAME_DATA_S12M_TIMECODE,
> > I'm following it as format is same, but change m to & 3 for I think it's more simple.
> > 
> > 
> >>
> >>>>> +        av_log(ctx, AV_LOG_ERROR, "invalid data");
> >>>>> +        return;
> >>>>> +    }
> >>>>> +
> >>>>> +    for (int j = 1; j <= m; j++) {
> >>>>> +        char tcbuf[AV_TIMECODE_STR_SIZE];
> >>>>> +        av_timecode_make_smpte_tc_string(tcbuf, tc[j], 0);
> >>>>> +        av_log(ctx, AV_LOG_INFO, "timecode - %s%s", tcbuf, j != m ? ", " : "");
> >>>>> +    }
> >>>>> +}
> >>>>> +
> >>
> >> - Andreas
> >>
> >> [1]: https://www.smpte.org/free-standards-and-publications
> >> _______________________________________________
> >> 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".
Limin Wang June 29, 2020, 2:58 p.m. UTC | #7
On Mon, Jun 29, 2020 at 03:52:00PM +0200, Andreas Rheinhardt wrote:
> lance.lmwang@gmail.com:
> > On Mon, Jun 29, 2020 at 02:06:49PM +0200, Andreas Rheinhardt wrote:
> >> lance.lmwang@gmail.com:
> >>> On Mon, Jun 29, 2020 at 04:53:21AM +0200, Andreas Rheinhardt wrote:
> >>>> lance.lmwang@gmail.com:
> >>>>> From: Limin Wang <lance.lmwang@gmail.com>
> >>>>>
> >>>>> Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> >>>>> ---
> >>>>>  doc/APIchanges        |  3 +++
> >>>>>  libavcodec/avpacket.c |  1 +
> >>>>>  libavcodec/decode.c   |  1 +
> >>>>>  libavcodec/packet.h   |  8 ++++++++
> >>>>>  libavcodec/version.h  |  2 +-
> >>>>>  libavformat/dump.c    | 22 ++++++++++++++++++++++
> >>>>>  6 files changed, 36 insertions(+), 1 deletion(-)
> >>>>>
> >>>>> diff --git a/doc/APIchanges b/doc/APIchanges
> >>>>> index 1d6cc36..7cad7fa 100644
> >>>>> --- a/doc/APIchanges
> >>>>> +++ b/doc/APIchanges
> >>>>> @@ -15,6 +15,9 @@ libavutil:     2017-10-21
> >>>>>  
> >>>>>  API changes, most recent first:
> >>>>>  
> >>>>> +2020-06-xx - xxxxxxxxxx - lavc 58.94.100 - packet.h
> >>>>> +  Add AV_PKT_DATA_S12M_TIMECODE.
> >>>>> +
> >>>>>  2020-06-12 - b09fb030c1 - lavu 56.55.100 - pixdesc.h
> >>>>>    Add AV_PIX_FMT_X2RGB10.
> >>>>>  
> >>>>> diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
> >>>>> index dce26cb..a4316a7 100644
> >>>>> --- a/libavcodec/avpacket.c
> >>>>> +++ b/libavcodec/avpacket.c
> >>>>> @@ -400,6 +400,7 @@ const char *av_packet_side_data_name(enum AVPacketSideDataType type)
> >>>>>      case AV_PKT_DATA_PRFT:                       return "Producer Reference Time";
> >>>>>      case AV_PKT_DATA_ICC_PROFILE:                return "ICC Profile";
> >>>>>      case AV_PKT_DATA_DOVI_CONF:                  return "DOVI configuration record";
> >>>>> +    case AV_PKT_DATA_S12M_TIMECODE:              return "SMPTE 12-1 timecode";
> >>>>>      }
> >>>>>      return NULL;
> >>>>>  }
> >>>>> diff --git a/libavcodec/decode.c b/libavcodec/decode.c
> >>>>> index a4e50c0..a2bf0cc 100644
> >>>>> --- a/libavcodec/decode.c
> >>>>> +++ b/libavcodec/decode.c
> >>>>> @@ -1699,6 +1699,7 @@ int ff_decode_frame_props(AVCodecContext *avctx, AVFrame *frame)
> >>>>>          { AV_PKT_DATA_CONTENT_LIGHT_LEVEL,        AV_FRAME_DATA_CONTENT_LIGHT_LEVEL },
> >>>>>          { AV_PKT_DATA_A53_CC,                     AV_FRAME_DATA_A53_CC },
> >>>>>          { AV_PKT_DATA_ICC_PROFILE,                AV_FRAME_DATA_ICC_PROFILE },
> >>>>> +        { AV_PKT_DATA_S12M_TIMECODE,              AV_FRAME_DATA_S12M_TIMECODE },
> >>>>>      };
> >>>>>  
> >>>>>      if (pkt) {
> >>>>> diff --git a/libavcodec/packet.h b/libavcodec/packet.h
> >>>>> index 41485f4..72cb4f7 100644
> >>>>> --- a/libavcodec/packet.h
> >>>>> +++ b/libavcodec/packet.h
> >>>>> @@ -283,6 +283,14 @@ enum AVPacketSideDataType {
> >>>>>      AV_PKT_DATA_DOVI_CONF,
> >>>>>  
> >>>>>      /**
> >>>>> +     * Timecode which conforms to SMPTE ST 12-1. The data is an array of 4 uint32_t
> >>>>> +     * where the first uint32_t describes how many (1-3) of the other timecodes are used.
> >>>>> +     * The timecode format is described in the av_timecode_get_smpte_from_framenum()
> >>>>> +     * function in libavutil/timecode.c.
> >>>>
> >>>> The format is documented in a C source file, not a public header? Sure
> >>>> about that?
> >>>
> >>> For AV_PKT_DATA_S12M_TIMECODE use the same format with AV_FRAME_DATA_S12M_TIMECODE, so I copy the
> >>> comments for AV_FRAME_DATA_S12M_TIMECODE, it's not my comments for it. I think the format is described
> >>> in "SMPTE ST 12-2:2014" standard document, but it's not free for download.
> >>>
> >>
> >> It's SMPTE ST 12-1:2014 and this standard is currently (until 21 July)
> >> available for free [1] due to Corona.
> > 
> > oh, it's good to hear it's free due to Corona. I haven't notice it's online.
> > 
> >>
> >>> What's your suggestion to change?
> >>>
> >>
> >> The best is probably to incorporate the documentation of the format into
> >> the documentation of av_timecode_get_smpte_from_framenum.
> > OK, I'll remove the comments for the timecode format.
> >>
> >>>>
> >>>>> +     */
> >>>>> +    AV_PKT_DATA_S12M_TIMECODE,
> >>>>> +
> >>>>> +    /**
> >>>>>       * The number of side data types.
> >>>>>       * This is not part of the public API/ABI in the sense that it may
> >>>>>       * change when new side data types are added.
> >>>>> diff --git a/libavcodec/version.h b/libavcodec/version.h
> >>>>> index 0359302..482cc6d 100644
> >>>>> --- a/libavcodec/version.h
> >>>>> +++ b/libavcodec/version.h
> >>>>> @@ -28,7 +28,7 @@
> >>>>>  #include "libavutil/version.h"
> >>>>>  
> >>>>>  #define LIBAVCODEC_VERSION_MAJOR  58
> >>>>> -#define LIBAVCODEC_VERSION_MINOR  93
> >>>>> +#define LIBAVCODEC_VERSION_MINOR  94
> >>>>>  #define LIBAVCODEC_VERSION_MICRO 100
> >>>>>  
> >>>>>  #define LIBAVCODEC_VERSION_INT  AV_VERSION_INT(LIBAVCODEC_VERSION_MAJOR, \
> >>>>> diff --git a/libavformat/dump.c b/libavformat/dump.c
> >>>>> index 117c681..c5f0bd6 100644
> >>>>> --- a/libavformat/dump.c
> >>>>> +++ b/libavformat/dump.c
> >>>>> @@ -34,6 +34,7 @@
> >>>>>  #include "libavutil/replaygain.h"
> >>>>>  #include "libavutil/spherical.h"
> >>>>>  #include "libavutil/stereo3d.h"
> >>>>> +#include "libavutil/timecode.h"
> >>>>>  
> >>>>>  #include "avformat.h"
> >>>>>  
> >>>>> @@ -402,6 +403,23 @@ static void dump_dovi_conf(void *ctx, AVPacketSideData* sd)
> >>>>>             dovi->dv_bl_signal_compatibility_id);
> >>>>>  }
> >>>>>  
> >>>>> +static void dump_s12m_timecode(void *ctx, AVPacketSideData* sd)
> >>>>> +{
> >>>>> +    uint32_t *tc = (uint32_t*)sd->data;
> >>>>> +    int m = tc[0] & 3;
> >>>>> +
> >>>>> +    if (sd->size != sizeof(uint32_t) * 4) {
> >>
> >> You already dereferenced tc before this check which might crash if the
> >> size is actually < sizeof(uint32_t). And why do you only look at the
> >> lowest two bits of m? Is this designed with future extensions of the
> >> timecode in mind? But even then one can't simply use the upper 30 bits
> >> to store something else, because that has not been documented.
> >> (Moreover, if one allowed extensions, one should allow sd->size to be >=
> >> sizeof(uint32_t) * 4.)
> > 
> > It's from the comments for the side data, it used to avoid index overflow,
> > for the array is 4. I'll fix to get the m after the checking.
> 
> I know it is to ensure that you don't overread. I'd simply consider it
> invalid data if the count were > 3.

Andreas, please help to review the updated v3 version, hopes it solves all of your
comments.

> 
> > 
> > comments:
> > where the first uint32_t describes how many (1-3) of the other timecodes are used
> > 
> > I think it's for the timecode SEI define 2bits for the count of used timecomes,
> > it's not related to SMPTE ST 12-1. vf_showinfo.c have dump AV_FRAME_DATA_S12M_TIMECODE,
> > I'm following it as format is same, but change m to & 3 for I think it's more simple.
> > 
> > 
> >>
> >>>>> +        av_log(ctx, AV_LOG_ERROR, "invalid data");
> >>>>> +        return;
> >>>>> +    }
> >>>>> +
> >>>>> +    for (int j = 1; j <= m; j++) {
> >>>>> +        char tcbuf[AV_TIMECODE_STR_SIZE];
> >>>>> +        av_timecode_make_smpte_tc_string(tcbuf, tc[j], 0);
> >>>>> +        av_log(ctx, AV_LOG_INFO, "timecode - %s%s", tcbuf, j != m ? ", " : "");
> >>>>> +    }
> >>>>> +}
> >>>>> +
> >>
> >> - Andreas
> >>
> >> [1]: https://www.smpte.org/free-standards-and-publications
> >> _______________________________________________
> >> 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/doc/APIchanges b/doc/APIchanges
index 1d6cc36..7cad7fa 100644
--- a/doc/APIchanges
+++ b/doc/APIchanges
@@ -15,6 +15,9 @@  libavutil:     2017-10-21
 
 API changes, most recent first:
 
+2020-06-xx - xxxxxxxxxx - lavc 58.94.100 - packet.h
+  Add AV_PKT_DATA_S12M_TIMECODE.
+
 2020-06-12 - b09fb030c1 - lavu 56.55.100 - pixdesc.h
   Add AV_PIX_FMT_X2RGB10.
 
diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
index dce26cb..a4316a7 100644
--- a/libavcodec/avpacket.c
+++ b/libavcodec/avpacket.c
@@ -400,6 +400,7 @@  const char *av_packet_side_data_name(enum AVPacketSideDataType type)
     case AV_PKT_DATA_PRFT:                       return "Producer Reference Time";
     case AV_PKT_DATA_ICC_PROFILE:                return "ICC Profile";
     case AV_PKT_DATA_DOVI_CONF:                  return "DOVI configuration record";
+    case AV_PKT_DATA_S12M_TIMECODE:              return "SMPTE 12-1 timecode";
     }
     return NULL;
 }
diff --git a/libavcodec/decode.c b/libavcodec/decode.c
index a4e50c0..a2bf0cc 100644
--- a/libavcodec/decode.c
+++ b/libavcodec/decode.c
@@ -1699,6 +1699,7 @@  int ff_decode_frame_props(AVCodecContext *avctx, AVFrame *frame)
         { AV_PKT_DATA_CONTENT_LIGHT_LEVEL,        AV_FRAME_DATA_CONTENT_LIGHT_LEVEL },
         { AV_PKT_DATA_A53_CC,                     AV_FRAME_DATA_A53_CC },
         { AV_PKT_DATA_ICC_PROFILE,                AV_FRAME_DATA_ICC_PROFILE },
+        { AV_PKT_DATA_S12M_TIMECODE,              AV_FRAME_DATA_S12M_TIMECODE },
     };
 
     if (pkt) {
diff --git a/libavcodec/packet.h b/libavcodec/packet.h
index 41485f4..72cb4f7 100644
--- a/libavcodec/packet.h
+++ b/libavcodec/packet.h
@@ -283,6 +283,14 @@  enum AVPacketSideDataType {
     AV_PKT_DATA_DOVI_CONF,
 
     /**
+     * Timecode which conforms to SMPTE ST 12-1. The data is an array of 4 uint32_t
+     * where the first uint32_t describes how many (1-3) of the other timecodes are used.
+     * The timecode format is described in the av_timecode_get_smpte_from_framenum()
+     * function in libavutil/timecode.c.
+     */
+    AV_PKT_DATA_S12M_TIMECODE,
+
+    /**
      * The number of side data types.
      * This is not part of the public API/ABI in the sense that it may
      * change when new side data types are added.
diff --git a/libavcodec/version.h b/libavcodec/version.h
index 0359302..482cc6d 100644
--- a/libavcodec/version.h
+++ b/libavcodec/version.h
@@ -28,7 +28,7 @@ 
 #include "libavutil/version.h"
 
 #define LIBAVCODEC_VERSION_MAJOR  58
-#define LIBAVCODEC_VERSION_MINOR  93
+#define LIBAVCODEC_VERSION_MINOR  94
 #define LIBAVCODEC_VERSION_MICRO 100
 
 #define LIBAVCODEC_VERSION_INT  AV_VERSION_INT(LIBAVCODEC_VERSION_MAJOR, \
diff --git a/libavformat/dump.c b/libavformat/dump.c
index 117c681..c5f0bd6 100644
--- a/libavformat/dump.c
+++ b/libavformat/dump.c
@@ -34,6 +34,7 @@ 
 #include "libavutil/replaygain.h"
 #include "libavutil/spherical.h"
 #include "libavutil/stereo3d.h"
+#include "libavutil/timecode.h"
 
 #include "avformat.h"
 
@@ -402,6 +403,23 @@  static void dump_dovi_conf(void *ctx, AVPacketSideData* sd)
            dovi->dv_bl_signal_compatibility_id);
 }
 
+static void dump_s12m_timecode(void *ctx, AVPacketSideData* sd)
+{
+    uint32_t *tc = (uint32_t*)sd->data;
+    int m = tc[0] & 3;
+
+    if (sd->size != sizeof(uint32_t) * 4) {
+        av_log(ctx, AV_LOG_ERROR, "invalid data");
+        return;
+    }
+
+    for (int j = 1; j <= m; j++) {
+        char tcbuf[AV_TIMECODE_STR_SIZE];
+        av_timecode_make_smpte_tc_string(tcbuf, tc[j], 0);
+        av_log(ctx, AV_LOG_INFO, "timecode - %s%s", tcbuf, j != m ? ", " : "");
+    }
+}
+
 static void dump_sidedata(void *ctx, AVStream *st, const char *indent)
 {
     int i;
@@ -468,6 +486,10 @@  static void dump_sidedata(void *ctx, AVStream *st, const char *indent)
             av_log(ctx, AV_LOG_INFO, "DOVI configuration record: ");
             dump_dovi_conf(ctx, &sd);
             break;
+        case AV_PKT_DATA_S12M_TIMECODE:
+            av_log(ctx, AV_LOG_INFO, "S12M timecode: ");
+            dump_s12m_timecode(ctx, &sd);
+            break;
         default:
             av_log(ctx, AV_LOG_INFO,
                    "unknown side data type %d (%d bytes)", sd.type, sd.size);