[FFmpeg-devel] [PATCH 04/21] libavcodec: added a structure AVData for decoding timed metadata

[Nicolas George]

Le septidi 7 fructidor, an CCXXIV, erkki.seppala.ext at nokia.com a écrit :
> From: Erkki Seppälä <erkki.seppala.ext at nokia.com>
> 
> Also added avdata_alloc and avdata_free for dealing with it. AVData
> can contain arbitrary binary data and comes with a format-field so far
> unused.
> 
> The purpose is that AVMEDIA_TYPE_DATA -kind codecs can store frames in
> this format.

Please consider using AVFrame itself instead. It has all the necessary
fields. It has many more fields, but is it really an issue?

The strongest case for using AVFrame is uniformity. Thanks to wm4, we now
have the same API for audio and video decoding. With that, code can be
written to manipulate frames in a generic manner. Timed metadata would be
handled the same way without extra code.

Integration in libavfilter would be easier too.

Below are comments assuming you reject that idea:

> 
> Signed-off-by: Erkki Seppälä <erkki.seppala.ext at nokia.com>
> Signed-off-by: OZOPlayer <OZOPL at nokia.com>
> ---
>  libavcodec/avcodec.h | 20 ++++++++++++++++++++
>  libavcodec/utils.c   | 19 +++++++++++++++++++
>  2 files changed, 39 insertions(+)
> 
> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> index 6ac6646..fb8f363 100644
> --- a/libavcodec/avcodec.h
> +++ b/libavcodec/avcodec.h
> @@ -3934,6 +3934,14 @@ typedef struct AVSubtitle {
>      int64_t pts;    ///< Same as packet pts, in AV_TIME_BASE
>  } AVSubtitle;
>  
> +typedef struct AVData {

> +    uint16_t format;

Using a small type seems like a micro-optimization. Not even a real one:
because of alignment requirements, this field will actually use 8 octets
(remember to put smaller fields after bigger ones), and type promotions will
cost time.

I suggest to use a normal int field. That has the possibility of leaving
gaps in the values or using 4ccs.

>                       /** 0 = timed metadata */

An enum would be better IMHO.

> +    int64_t pts;
> +    int64_t dts;
> +

> +    AVBufferRef *data;

Since it uses refcounted buffers, the API to create a new reference to the
same buffer seems missing.

> +} AVData;
> +
>  /**
>   * This struct describes the properties of an encoded stream.
>   *
> @@ -4317,6 +4325,18 @@ int avcodec_close(AVCodecContext *avctx);
>  void avsubtitle_free(AVSubtitle *sub);
>  
>  /**
> + * Allocate an empty data (typically use with timed metadata)
> + */

> +AVData *avdata_alloc(void);

I say it again: I would prefer if these APIs returned an error code in case
of failure. Even if the error code can only be ENOMEM, it is better than
hardcoding it at every call site.

> +
> +/**
> + * Free all allocated data in the given data struct.
> + *

> + * @param sub AVData to free.

Well said, Captain Obvious ;-)

> + */
> +void avdata_free(AVData *sub);
> +
> +/**
>   * @}
>   */
>  
> diff --git a/libavcodec/utils.c b/libavcodec/utils.c
> index 138125a..dabe97e 100644
> --- a/libavcodec/utils.c
> +++ b/libavcodec/utils.c
> @@ -2955,6 +2955,25 @@ int attribute_align_arg avcodec_receive_packet(AVCodecContext *avctx, AVPacket *
>      return 0;
>  }
>  
> +AVData *avdata_alloc(void)
> +{
> +    AVData *data = av_mallocz(sizeof(*data));
> +

> +    if (!data)
> +        return NULL;

Seems redundant.

> +
> +    return data;
> +}
> +
> +void avdata_free(AVData *data)
> +{
> +    av_buffer_unref(&data->data);
> +

> +    memset(data, 0, sizeof(AVData));

Seems useless.

> +
> +    av_free(data);
> +}
> +
>  av_cold int avcodec_close(AVCodecContext *avctx)
>  {
>      int i;

Regards,

-- 
  Nicolas George
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20160823/f051365e/attachment.sig>