[FFmpeg-devel] [PATCH 1/2] allow passing subtitles header between decoder and encoder
Aurelien Jacobs
aurel
Wed Nov 10 01:21:12 CET 2010
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.
For example, the SubRip decoder don't expect any extradata (there is no
header in the SubRip format), but still it has to generate a
subtitle_header.
So the new flag still seems to be the best solution to me.
Any other idea ?
Aurel
More information about the ffmpeg-devel
mailing list