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

[Baptiste Coudurier]

Reimar D?ffinger wrote:
> 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?

Well, what you say is not false.
I just said "should" because I thought id was already set to
CODEC_ID_DVD_SUBTITLE, but this is not the case.

Yes the "if (format..." is 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.

Yes, I tend to agree that the codec_id/codec_type code is messy, but we
have to deal with the crappy fourcc people put in .mov/.mp4/.3gp etc...,
not mentioning that Apple use 'raw ' for both audio and video and that
'mp4s' is also used in .wmv.

This will be cleaned up when stsd parsing is done after reading the
atom. When this will be the case, codec_type will be known for sure and
this will simplify the code I think.

> 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.

Because CODEC_TYPE_SUBTITLE is set after matching the "fourcc", except
for this specific case (nero subs) because they use and special handler
type named "subp". Tracks are set to CODEC_TYPE_DATA by default and type
is updated after reading handler type. If handler type is fixed and
determined for 'tx3g' and 'text', then code in handler (hdlr) might be
updated to set type earlier.

-- 
Baptiste COUDURIER                              GnuPG Key Id: 0x5C1ABAAA
Key fingerprint                 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA
checking for life_signs in -lkenny... no
FFmpeg maintainer                                  http://www.ffmpeg.org