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

[Andreas Rheinhardt]

lance.lmwang at gmail.com:
> On Mon, Jun 29, 2020 at 04:53:21AM +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>
>>> ---
>>>  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