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

[Reimar Döffinger]

On Wed, Apr 01, 2009 at 11:39:46AM -0700, Baptiste Coudurier wrote:
> Yes the "if (format..." is ok to apply.

Done.

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

Hm, I would have described it as "the code assumed there are only
CODEC_TYPE_AUDIO, CODEC_TYPE_VIDEO and CODEC_TYPE_DATA and was not
updated when CODEC_TYPE_SUBTITLE was added".
At least that's what the comment on this line looks like:
> } else if (st->codec->codec_type != CODEC_TYPE_AUDIO && /* do not overwrite codec type */

The code basically looks like this now:
search audio id
if (!is VIDEO && id found)
  is AUDIO
else if (!is AUDIO) {
  search video id
  if (id found) is VIDEO
  else if (is DATA) {
    search sub id
    if (id found) is SUB
  }
}

While IMHO what was _meant_ is
if (is AUDIO or is DATA) {
  search audio id
  if (id found) is AUDIO
}
if (is VIDEO or is DATA) {
  search video id
  if (id found) is VIDEO
}
if (is SUB or is DATA) {
  search sub id
  if (id found) is SUB
}

or am I missing something?
I just meant to say, if I am missing something why my second version
would not be correct that should be explained by a comment.
If it is correct, are you interested in a patch to change it like that?
Note that I think the current code will do funny stuff for a (probably
invalid file) where e.g. handler == "vide" and format == "QDM2", you will
end up with a video stream with CODEC_ID_QDM2, which I think is about
the silliest way to handle it :-P ...