[FFmpeg-devel] [PATCH] vobsub in mov support

[Reimar Döffinger]

On Wed, Apr 01, 2009 at 09:32:13AM -0700, Baptiste Coudurier wrote:
> On 4/1/2009 2:43 AM, Reimar D?ffinger wrote:
> > On Wed, Apr 01, 2009 at 12:54:10AM -0700, Baptiste Coudurier wrote:
> >>> @@ -997,6 +998,7 @@
> >>>              // ttxt stsd contains display flags, justification, background
> >>>              // color, fonts, and default styles, so fake an atom to read it
> >>>              MOVAtom fake_atom = { .size = size - (url_ftell(pb) - start_pos) };
> >>> +            if (id != CODEC_ID_DVD_SUBTITLE) // this contains a proper esds atom
> >>>              mov_read_glbl(c, pb, fake_atom);
> >>>              st->codec->codec_id= id;
> >>>              st->codec->width = sc->width;
> >> This hunk should be sufficient
> > 
> > No, certainly not. 
> 
> Yes it is, modulo changing the test to if (format != AV_RL32("mp4s"))

Then it is not "this hunk" anymore in my view. I did say in the original
mail already that it is enough when changing the condition (though I did
not think of changing it to a test for format), so I did not expect your
reply to mean that.
So is adding that "if (format..." ok to apply?

> > ff_codec_movsubtitle_tags is only consulted for
> > CODEC_TYPE_DATA, never for CODEC_TYPE_SUBTITLE, and in addition it is
> > also skipped explicitly for mp4s (without that it would be overridden
> > from CODEC_TYPE_SUBTITLE to CODEC_TYPE_VIDEO).
> > The whole code is either very carefully crafted and severely lacking
> > documentation or a very big piece of nonsense.
> 
> Please spare me your comments when you don't know the code.

You're free to ignore my comments.
In case you choose not to I'd like to clarify:
By "the whole code" I meant lines 791 to 809 only, sorry for that and for
exaggerating if either of that annoyed you.
And I do think there is no need to know the code to conclude that when for
CODEC_TYPE_SUBTITLE all of codec_movaudio_tags, codec_movvideo_tags and
codec_bmp_tags are checked for matches but ff_codec_movsubtitle_tags is not
there either is a bug or it should be documented why.