[FFmpeg-devel] [PATCH] lavc: add ff_bprint_to_extradata() helper and use it.

[Clément Bœsch]

On Sun, Dec 30, 2012 at 02:52:08PM +0100, Nicolas George wrote:
> Le nonidi 9 nivôse, an CCXXI, Clement Boesch a écrit :
> > At the same time, it should fix 'warning: dereferencing type-punned
> > pointer will break strict-aliasing rules' warning for compilers who
> > don't consider uint8_t** and char** compatibles.
> > ---
> >  libavcodec/dvdsubenc.c     |  5 +++--
> >  libavcodec/internal.h      |  6 ++++++
> >  libavcodec/utils.c         | 14 ++++++++++++++
> >  libavformat/assdec.c       |  8 +++-----
> >  libavformat/jacosubdec.c   |  8 +++++---
> >  libavformat/samidec.c      |  9 +++------
> >  libavformat/subviewerdec.c |  8 +++-----
> >  7 files changed, 37 insertions(+), 21 deletions(-)
> > 
> > diff --git a/libavcodec/dvdsubenc.c b/libavcodec/dvdsubenc.c
> > index 2e0c37d..352272d 100644
> > --- a/libavcodec/dvdsubenc.c
> > +++ b/libavcodec/dvdsubenc.c
> > @@ -20,6 +20,7 @@
> >   */
> >  #include "avcodec.h"
> >  #include "bytestream.h"
> > +#include "internal.h"
> >  #include "libavutil/avassert.h"
> >  #include "libavutil/bprint.h"
> >  #include "libavutil/imgutils.h"
> > @@ -408,9 +409,9 @@ static int dvdsub_init(AVCodecContext *avctx)
> >          av_bprintf(&extradata, " %06"PRIx32"%c",
> >                     dvdc->global_palette[i] & 0xFFFFFF, i < 15 ? ',' : '\n');
> >  
> > -    if ((ret = av_bprint_finalize(&extradata, (char **)&avctx->extradata)) < 0)
> > +    ret = ff_bprint_to_extradata(avctx, &extradata);
> > +    if (ret < 0)
> >          return ret;
> > -    avctx->extradata_size = extradata.len;
> >  
> >      return 0;
> >  }
> > diff --git a/libavcodec/internal.h b/libavcodec/internal.h
> > index 2d3433f..69210a3 100644
> > --- a/libavcodec/internal.h
> > +++ b/libavcodec/internal.h
> > @@ -26,6 +26,7 @@
> >  
> >  #include <stdint.h>
> >  
> 
> > +#include "libavutil/bprint.h"
> >  #include "libavutil/mathematics.h"
> >  #include "libavutil/pixfmt.h"
> >  #include "avcodec.h"
> > @@ -201,4 +202,9 @@ int ff_codec_open2_recursive(AVCodecContext *avctx, const AVCodec *codec, AVDict
> >   */
> >  int ff_codec_close_recursive(AVCodecContext *avctx);
> >  
> > +/**
> > + * Finalize buf into extradata and set its size appropriately.
> > + */
> > +int ff_bprint_to_extradata(AVCodecContext *avctx, AVBPrint *buf);
> 
> For this kind of situation, I believe it is better to use the structure
> name:
> 
> int ff_bprint_to_extradata(AVCodecContext *avctx, struct AVBPrint *buf);
> 
> It reduces the inter-header dependencies and recompilation hell.
> 

Changed locally.

> > +
> >  #endif /* AVCODEC_INTERNAL_H */
> > diff --git a/libavcodec/utils.c b/libavcodec/utils.c
> > index b366995..7c7e502 100644
> > --- a/libavcodec/utils.c
> > +++ b/libavcodec/utils.c
> > @@ -2684,3 +2684,17 @@ int avcodec_is_open(AVCodecContext *s)
> >  {
> >      return !!s->internal;
> >  }
> > +
> > +int ff_bprint_to_extradata(AVCodecContext *avctx, AVBPrint *buf)
> > +{
> > +    int ret;
> > +    char *str;
> > +
> > +    ret = av_bprint_finalize(buf, &str);
> > +    if (ret < 0)
> > +        return ret;
> 
> > +    avctx->extradata = str;
> 
> avctx->extradata is supposed to be padded to FF_INPUT_BUFFER_PADDING_SIZE,
> IIRC.
> 
> > +    /* extradata needs to contain '\0' in case it is copied around */
> > +    avctx->extradata_size = buf->len + 1;
> 
> For the codecs I know (at least ASS- and dvdsub-in-Matroska), the
> terminating NUL is not part of the extradata, and therefore the +1 is wrong.
> If you know codecs where the terminating NUL should be included, a flag is
> probably the answer.
> 
> > +    return 0;
> > +}
> > diff --git a/libavformat/assdec.c b/libavformat/assdec.c
> > index 6c4614e..34229cb 100644
> > --- a/libavformat/assdec.c
> > +++ b/libavformat/assdec.c
> > @@ -22,6 +22,7 @@
> >  #include "avformat.h"
> >  #include "internal.h"
> >  #include "subtitles.h"
> > +#include "libavcodec/internal.h"
> >  #include "libavutil/bprint.h"
> >  
> >  typedef struct ASSContext{
> > @@ -132,12 +133,9 @@ static int ass_read_header(AVFormatContext *s)
> >  
> >      av_bprint_finalize(&line, NULL);
> >  
> > -    av_bprint_finalize(&header, (char **)&st->codec->extradata);
> > -    if (!st->codec->extradata) {
> > -        res = AVERROR(ENOMEM);
> > +    res = ff_bprint_to_extradata(st->codec, &header);
> > +    if (res < 0)
> >          goto end;
> > -    }
> 
> > -    st->codec->extradata_size = header.len + 1;
> 
> The "+1" was wrong.
> 
> (Strangely, I can not see its effect in the resulting MKV file; the Matroska
> muxer is probably doing some magic too.)
> 

Yeah right, I just fixed that.

> >  
> >      ff_subtitles_queue_finalize(&ass->q);
> >  
> > diff --git a/libavformat/jacosubdec.c b/libavformat/jacosubdec.c
> > index 1c58e3b..153da42 100644
> > --- a/libavformat/jacosubdec.c
> > +++ b/libavformat/jacosubdec.c
> > @@ -28,6 +28,7 @@
> >  #include "avformat.h"
> >  #include "internal.h"
> >  #include "subtitles.h"
> > +#include "libavcodec/internal.h"
> >  #include "libavcodec/jacosub.h"
> >  #include "libavutil/avstring.h"
> >  #include "libavutil/bprint.h"
> > @@ -159,7 +160,7 @@ static int jacosub_read_header(AVFormatContext *s)
> >      JACOsubContext *jacosub = s->priv_data;
> >      int shift_set = 0; // only the first shift matters
> >      int merge_line = 0;
> > -    int i;
> > +    int i, ret;
> >  
> >      AVStream *st = avformat_new_stream(s, NULL);
> >      if (!st)
> > @@ -228,8 +229,9 @@ static int jacosub_read_header(AVFormatContext *s)
> >      }
> >  
> >      /* general/essential directives in the extradata */
> > -    av_bprint_finalize(&header, (char **)&st->codec->extradata);
> 
> > -    st->codec->extradata_size = header.len + 1;
> 
> Do you know any container that can have jacosub in it?
> 

Except the standalone file, no, same answer for the others.

[...]

Applied with some changes, thanks.

-- 
Clément B.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 490 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20121230/2e8f8cf9/attachment.asc>