diff mbox

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

Message ID 1471943019-14136-5-git-send-email-erkki.seppala.ext@nokia.com
State Superseded
Headers show

Commit Message

erkki.seppala.ext@nokia.com Aug. 23, 2016, 9:03 a.m. UTC
From: Erkki Seppälä <erkki.seppala.ext@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.

Signed-off-by: Erkki Seppälä <erkki.seppala.ext@nokia.com>
Signed-off-by: OZOPlayer <OZOPL@nokia.com>
---
 libavcodec/avcodec.h | 20 ++++++++++++++++++++
 libavcodec/utils.c   | 19 +++++++++++++++++++
 2 files changed, 39 insertions(+)

Comments

Nicolas George Aug. 23, 2016, 3:36 p.m. UTC | #1
Le septidi 7 fructidor, an CCXXIV, erkki.seppala.ext@nokia.com a écrit :
> From: Erkki Seppälä <erkki.seppala.ext@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@nokia.com>
> Signed-off-by: OZOPlayer <OZOPL@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,
Paul B Mahol Aug. 23, 2016, 4:07 p.m. UTC | #2
On 8/23/16, Nicolas George <george@nsup.org> wrote:
> Le septidi 7 fructidor, an CCXXIV, erkki.seppala.ext@nokia.com a écrit :
>> From: Erkki Seppälä <erkki.seppala.ext@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?

Yes, please use AVFrame.

>
> 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.
Michael Niedermayer Aug. 23, 2016, 4:13 p.m. UTC | #3
On Tue, Aug 23, 2016 at 06:07:44PM +0200, Paul B Mahol wrote:
> On 8/23/16, Nicolas George <george@nsup.org> wrote:
> > Le septidi 7 fructidor, an CCXXIV, erkki.seppala.ext@nokia.com a écrit :
> >> From: Erkki Seppälä <erkki.seppala.ext@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?
> 
> Yes, please use AVFrame.

+1

[...]
erkki.seppala.ext@nokia.com Aug. 24, 2016, 8:30 a.m. UTC | #4
AVFrame certainly sounds a good solution. For AVData I followed the lead 
from AVSubtitle as they are quite similar concepts. And AVFrame did 
really look quite a big struct ;).

I'll rework the code to replace AVData with AVFrame.

Thanks for review!
diff mbox

Patch

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; /** 0 = timed metadata */
+    int64_t pts;
+    int64_t dts;
+
+    AVBufferRef *data;
+} 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);
+
+/**
+ * Free all allocated data in the given data struct.
+ *
+ * @param sub AVData to free.
+ */
+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;
+
+    return data;
+}
+
+void avdata_free(AVData *data)
+{
+    av_buffer_unref(&data->data);
+
+    memset(data, 0, sizeof(AVData));
+
+    av_free(data);
+}
+
 av_cold int avcodec_close(AVCodecContext *avctx)
 {
     int i;