[FFmpeg-devel] [PATCH 1/2] allow passing subtitles header between decoder and encoder

Aurelien Jacobs aurel
Sat Nov 13 14:55:13 CET 2010


On Fri, Nov 12, 2010 at 08:25:12PM +0100, Michael Niedermayer wrote:
> On Wed, Nov 10, 2010 at 11:52:37PM +0100, Aurelien Jacobs wrote:
> > On Wed, Nov 10, 2010 at 03:52:13PM +0100, Michael Niedermayer wrote:
> > > On Wed, Nov 10, 2010 at 03:39:23PM +0100, Aurelien Jacobs wrote:
> > > > On Wed, Nov 10, 2010 at 03:06:22PM +0100, Michael Niedermayer wrote:
> > > > > On Wed, Nov 10, 2010 at 02:52:43PM +0100, Aurelien Jacobs wrote:
> > > > > > On Wed, Nov 10, 2010 at 04:30:17AM +0100, Michael Niedermayer wrote:
> > > > > > > On Wed, Nov 10, 2010 at 01:21:12AM +0100, Aurelien Jacobs wrote:
> > > > > > > > On Wed, Nov 10, 2010 at 12:36:01AM +0100, Michael Niedermayer wrote:
> > > > > > > > > On Sun, Nov 07, 2010 at 11:44:03PM +0100, Aurelien Jacobs wrote:
> > > > > > > > > > On Mon, Oct 25, 2010 at 03:24:18AM +0200, Michael Niedermayer wrote:
> > > > > > > > > > > On Mon, Oct 25, 2010 at 12:04:59AM +0200, Aurelien Jacobs wrote:
> > > > > > > > > > > > On Sat, Oct 23, 2010 at 09:57:26PM +0200, Michael Niedermayer wrote:
> > > > > > > > > > > > > On Tue, Oct 19, 2010 at 09:55:47PM +0200, Aurelien Jacobs wrote:
> > > > > > > > > > > > > > On Tue, Oct 19, 2010 at 03:43:52PM +0200, Michael Niedermayer wrote:
> > > > > > > > > > > > > > > On Sat, Oct 16, 2010 at 05:31:26PM +0200, Aurelien Jacobs wrote:
> > > > > > > > > > > > > > > > ---
> > > > > > > > > > > > > > > >  ffmpeg.c             |   11 +++++++++++
> > > > > > > > > > > > > > > >  libavcodec/avcodec.h |    8 ++++++++
> > > > > > > > > > > > > > > >  libavcodec/utils.c   |    2 ++
> > > > > > > > > > > > > > > >  libavformat/utils.c  |    4 ++++
> > > > > > > > > > > > > > > >  4 files changed, 25 insertions(+), 0 deletions(-)
> > > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > > diff --git a/ffmpeg.c b/ffmpeg.c
> > > > > > > > > > > > > > > > index 4f59b2e..7622ccc 100644
> > > > > > > > > > > > > > > > --- a/ffmpeg.c
> > > > > > > > > > > > > > > > +++ b/ffmpeg.c
> > > > > > > > > > > > > > > > @@ -2365,6 +2365,7 @@ static int transcode(AVFormatContext **output_files,
> > > > > > > > > > > > > > > >          ost = ost_table[i];
> > > > > > > > > > > > > > > >          if (ost->encoding_needed) {
> > > > > > > > > > > > > > > >              AVCodec *codec = i < nb_output_codecs ? output_codecs[i] : NULL;
> > > > > > > > > > > > > > > > +            AVCodecContext *dec = ist_table[ost->source_index]->st->codec;
> > > > > > > > > > > > > > > >              if (!codec)
> > > > > > > > > > > > > > > >                  codec = avcodec_find_encoder(ost->st->codec->codec_id);
> > > > > > > > > > > > > > > >              if (!codec) {
> > > > > > > > > > > > > > > > @@ -2373,6 +2374,15 @@ static int transcode(AVFormatContext **output_files,
> > > > > > > > > > > > > > > >                  ret = AVERROR(EINVAL);
> > > > > > > > > > > > > > > >                  goto dump_format;
> > > > > > > > > > > > > > > >              }
> > > > > > > > > > > > > > > > +            if (dec->subtitle_header) {
> > > > > > > > > > > > > > > > +                ost->st->codec->subtitle_header = av_malloc(dec->subtitle_header_size);
> > > > > > > > > > > > > > > > +                if (!ost->st->codec->subtitle_header) {
> > > > > > > > > > > > > > > > +                    ret = AVERROR(ENOMEM);
> > > > > > > > > > > > > > > > +                    goto dump_format;
> > > > > > > > > > > > > > > > +                }
> > > > > > > > > > > > > > > > +                memcpy(ost->st->codec->subtitle_header, dec->subtitle_header, dec->subtitle_header_size);
> > > > > > > > > > > > > > > > +                ost->st->codec->subtitle_header_size = dec->subtitle_header_size;
> > > > > > > > > > > > > > > > +            }
> > > > > > > > > > > > > > > >              if (avcodec_open(ost->st->codec, codec) < 0) {
> > > > > > > > > > > > > > > >                  snprintf(error, sizeof(error), "Error while opening encoder for output stream #%d.%d - maybe incorrect parameters such as bit_rate, rate, width or height",
> > > > > > > > > > > > > > > >                          ost->file_index, ost->index);
> > > > > > > > > > > > > > > > @@ -2728,6 +2738,7 @@ static int transcode(AVFormatContext **output_files,
> > > > > > > > > > > > > > > >                  }
> > > > > > > > > > > > > > > >                  av_fifo_free(ost->fifo); /* works even if fifo is not
> > > > > > > > > > > > > > > >                                               initialized but set to zero */
> > > > > > > > > > > > > > > > +                av_freep(&ost->st->codec->subtitle_header);
> > > > > > > > > > > > > > > >                  av_free(ost->pict_tmp.data[0]);
> > > > > > > > > > > > > > > >                  if (ost->video_resample)
> > > > > > > > > > > > > > > >                      sws_freeContext(ost->img_resample_ctx);
> > > > > > > > > > > > > > > > diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> > > > > > > > > > > > > > > > index 4bddbaa..ab3cbd9 100644
> > > > > > > > > > > > > > > > --- a/libavcodec/avcodec.h
> > > > > > > > > > > > > > > > +++ b/libavcodec/avcodec.h
> > > > > > > > > > > > > > > > @@ -2744,6 +2744,14 @@ typedef struct AVCodecContext {
> > > > > > > > > > > > > > > >       * - decoding: unused
> > > > > > > > > > > > > > > >       */
> > > > > > > > > > > > > > > >      int lpc_passes;
> > > > > > > > > > > > > > > > +
> > > > > > > > > > > > > > > > +    /**
> > > > > > > > > > > > > > > > +     * Header containing style information for text subtitles.
> > > > > > > > > > > > > > > > +     * - encoding: Set/allocated/freed by user (before avcodec_open())
> > > > > > > > > > > > > > > > +     * - decoding: Set/allocated/freed by libavcodec (by avcodec_open())
> > > > > > > > > > > > > > > > +     */
> > > > > > > > > > > > > > > > +    uint8_t *subtitle_header;
> > > > > > > > > > > > > > > > +    int subtitle_header_size;
> > > > > > > > > > > > > > > >  } AVCodecContext;
> > > > > > > > > > > > > > > >  
> > > > > > > > > > > > > > > >  /**
> > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > The description should be enough to implement a decoder and a user app and
> > > > > > > > > > > > > > > a subtitle renderer based on it. (some common sense and guessing not excluded)
> > > > > > > > > > > > > > > but this seems too terse for that
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > OK. Updated patch.
> > > > > > > > > > > > > > I think it should now be enough to implement anything related to this
> > > > > > > > > > > > > > field, with a little bit of common sense and the ASS spec around.
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > also are you sure every subtitle decoder will be able to set this by
> > > > > > > > > > > > > > > avcodec_open() ?
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > Yes, sure. It wouldn't be very useful anyway to set it later on.
> > > > > > > > > > > > > > I've already (started to) implement several decoders (SubRip,
> > > > > > > > > > > > > > MicroDVD, Movtext...), and they fits well within this model.
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > Aurel
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > [...]
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> > > > > > > > > > > > > > index 4bddbaa..99bc7ad 100644
> > > > > > > > > > > > > > --- a/libavcodec/avcodec.h
> > > > > > > > > > > > > > +++ b/libavcodec/avcodec.h
> > > > > > > > > > > > > > @@ -2744,6 +2744,17 @@ typedef struct AVCodecContext {
> > > > > > > > > > > > > >       * - decoding: unused
> > > > > > > > > > > > > >       */
> > > > > > > > > > > > > >      int lpc_passes;
> > > > > > > > > > > > > > +
> > > > > > > > > > > > > > +    /**
> > > > > > > > > > > > > > +     * Header containing style information for text subtitles.
> > > > > > > > > > > > > > +     * For SUBTITLE_ASS subtitle type, it should contain the whole ASS
> > > > > > > > > > > > > > +     * [Script Info] and [V4+ Styles] section, plus the [Events] line and
> > > > > > > > > > > > > > +     * the Format line following. It shouldn't include any Dialogue line.
> > > > > > > > > > > > > 
> > > > > > > > > > > > > 
> > > > > > > > > > > > > 
> > > > > > > > > > > > > > +     * - encoding: Set/allocated/freed by user (before avcodec_open())
> > > > > > > > > > > > > [...]
> > > > > > > > > > > > > > @@ -742,6 +742,8 @@ av_cold int avcodec_close(AVCodecContext *avctx)
> > > > > > > > > > > > > >      av_freep(&avctx->priv_data);
> > > > > > > > > > > > > >      if(avctx->codec && avctx->codec->encode)
> > > > > > > > > > > > > >          av_freep(&avctx->extradata);
> > > > > > > > > > > > > > +    if (avctx->codec && avctx->codec->encode)
> > > > > > > > > > > > > > +        av_freep(&avctx->subtitle_header);
> > > > > > > > > > > > > >      avctx->codec = NULL;
> > > > > > > > > > > > > >      entangled_thread_counter--;
> > > > > > > > > > > > > 
> > > > > > > > > > > > > doesnt match
> > > > > > > > > > > > 
> > > > > > > > > > > > Right !
> > > > > > > > > > > > To be consistent it seems better to let the application free the memory
> > > > > > > > > > > > it allocated, so I just removed this av_freep(). Anyway, there was
> > > > > > > > > > > > already the needed av_freep() in ffmpeg.
> > > > > > > > > > > > 
> > > > > > > > > > > > > > diff --git a/libavformat/utils.c b/libavformat/utils.c
> > > > > > > > > > > > > > index 8a08557..a137e69 100644
> > > > > > > > > > > > > > --- a/libavformat/utils.c
> > > > > > > > > > > > > > +++ b/libavformat/utils.c
> > > > > > > > > > > > > > @@ -2022,6 +2022,9 @@ static int has_codec_parameters(AVCodecContext *enc)
> > > > > > > > > > > > > >      case AVMEDIA_TYPE_VIDEO:
> > > > > > > > > > > > > >          val = enc->width && enc->pix_fmt != PIX_FMT_NONE;
> > > > > > > > > > > > > >          break;
> > > > > > > > > > > > > > +    case AVMEDIA_TYPE_SUBTITLE:
> > > > > > > > > > > > > > +        val = !!enc->subtitle_header;
> > > > > > > > > > > > > > +        break;
> > > > > > > > > > > > > >      default:
> > > > > > > > > > > > > >          val = 1;
> > > > > > > > > > > > > >          break;
> > > > > > > > > > > > > 
> > > > > > > > > > > > > This will slow every sub title decoder down that never sets this
> > > > > > > > > > > > 
> > > > > > > > > > > > The best way to fix this is probably to add a new capability flag to
> > > > > > > > > > > > codecs which use a subtitle_header, and check for this flag here.
> > > > > > > > > > > > Is this OK ?
> > > > > > > > > > > 
> > > > > > > > > > > if subtitle_header is part of the format then its the format (text/ass/bitmap)
> > > > > > > > > > > that decides if such header should be expected.
> > > > > > > > > > 
> > > > > > > > > > Kind of...
> > > > > > > > > > 
> > > > > > > > > > > But if that doesnt work out then a flag could be used too
> > > > > > > > > > 
> > > > > > > > > > Indeed, that doesn't work out. The format (text/ass/bitmap) is a
> > > > > > > > > > property of the resulting AVSubtitle, not of the AVCodec.
> > > > > > > > > > libavformat need this information before opening the codec, so
> > > > > > > > > > a flag in AVCodec seems the best solution to me.
> > > > > > > > > 
> > > > > > > > > hmpf, what about extradata != NULL then?
> > > > > > > > 
> > > > > > > > Don't work either. This has nothing to do with extradata.
> > > > > > > 
> > > > > > > extradata is the format specific global header subtitle_header is the global
> > > > > > > format unspecific header.
> > > > > > 
> > > > > > Exactly.
> > > > > > 
> > > > > > > If extradata is set where else would its content go or what does it do
> > > > > > > if not for subtitle_header ?
> > > > > > 
> > > > > > Well, it depends on the content of extradata.
> > > > > > 
> > > > > > For text subtitle, decoder should (almost?) always convert extradata to
> > > > > > subtitle_header. I can still imagine some cases where extradata contains
> > > > > > some informations that don't map to ASS subtitle_header, and which thus
> > > > > > would need to be integrated in each of the AVSubtitle. This case is
> > > > > > highly unlikely given the versatility of ASS and the general poverty of
> > > > > > most other formats.
> > > > > > For example, if a subtitle format allows to describe a generic
> > > > > > fade-in/fade-out effect, it can't be translated to ASS header, and would
> > > > > > thus have to be duplicated in each ASS dialog line.
> > > > > > 
> > > > > > For bitmap subtitle, I didn't check, but I guess that extradata can
> > > > > > contain a palette that is to be applied to each of the decoded bitmap,
> > > > > > but that won't generate any useful subtitle_header.
> > > > > > 
> > > > > > > if its not set subtitle_header could be set to a constant default by a
> > > > > > > decoder
> > > > > > 
> > > > > > That's right. But still, decoders which provide such a constant
> > > > > > subtitle_header (which will be different for each decoder) will need
> > > > > > to be flagged as such, so that av_find_stream_info() can get it.
> > > > > > 
> > > > > 
> > > > > > > or it could depend on packet content but in that case one wonders how global
> > > > > > > it is.
> > > > > > 
> > > > > > Indeed, if it depends on packet content it can't be considered as a
> > > > > > global header, and it shouldn't go into subtitle_header.
> > > > > 
> > > > > if so simply opening the decoder should be enough and av_find_stream_info()
> > > > > never has to wait for packets.
> > > > 
> > > > Exactly yes. That's how my patch works.
> > > > But av_find_stream_info() won't open the decoder unless
> > > > has_codec_parameters() return false.
> > > > 
> > > > > So if iam not missing something this shouldnt need a flag
> > > > 
> > > > The flag is needed so that has_codec_parameters() can tell if it is
> > > > worth to try to open the decoder or if we shouldn't even bother.
> > > > 
> > > > An alternative would be to force av_find_stream_info() to always open
> > > > the decoder even if it is not strictly needed, but it is clearly not
> > > > optimal.
> > > 
> > > always opening subtitle decoders seems like the logic solution
> > > in what case would that not be optimal?
> > 
> > It may force opening a decoder such as dvbsub which wouldn't require it
> > otherwise during av_find_stream_info(). But I guess this won't have any
> > noticeable effect.
> > 
> > > pure text decoders wont spend much time in open() and complex decoders will
> > > likely have a global header requireing to be opened anyway
> > 
> > Yes, I agree.
> > So this new patch don't have a new flag anymore. It's quite simpler,
> > and I like it.
> > 
> > Aurel
> >  ffmpeg.c             |   11 +++++++++++
> >  libavcodec/avcodec.h |   11 +++++++++++
> >  libavformat/utils.c  |    8 +++++++-
> >  3 files changed, 29 insertions(+), 1 deletion(-)
> > f119ab0016ac814376a224ae3fd23f8470988b3d  subtitle_header.diff
> > commit 575beccecad7b9b99f3f185cb53f7ba9250b48d8
> > Author: Aurelien Jacobs <aurel at gnuage.org>
> > Date:   Thu Aug 5 22:12:37 2010 +0200
> > 
> >     allow passing subtitles header between decoder and encoder
> 
> lgtm if tested

Applied.

Aurel



More information about the ffmpeg-devel mailing list