[FFmpeg-devel] [PATCH 1/2] allow passing subtitles header between decoder and encoder
Aurelien Jacobs
aurel
Wed Nov 10 14:52:43 CET 2010
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.
> nothing wrong with adding a flag, i just want to understand this a bit
> better...
OK.
Aurel
More information about the ffmpeg-devel
mailing list