| Message ID | 1471943019-14136-5-git-send-email-erkki.seppala.ext@nokia.com |
|---|---|
| State | Superseded |
| Headers | show |
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,
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.
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 [...]
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 --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;