[FFmpeg-devel] [PATCH 1/2] allow passing subtitles header between decoder and encoder
Michael Niedermayer
michaelni
Fri Nov 12 20:25:12 CET 2010
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
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Many that live deserve death. And some that die deserve life. Can you give
it to them? Then do not be too eager to deal out death in judgement. For
even the very wise cannot see all ends. -- Gandalf
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20101112/48a8f3ab/attachment.pgp>
More information about the ffmpeg-devel
mailing list