[MPlayer-dev-eng] [PATCH] libmpdemux/demux_audio: Skip ID3v2.4 tags.

[Benoît Thébaudeau]

Ingo,

> I personally would only perform a footer flag check and accept the
> 3DI footer
> if version is 4 or above.

Done (see attachment).

> AFAIK valid ID3v2 tag headers start with version 2, so we should not
> drop
> this check.

Done.

> You already mentioned that ("very, very unlikely to matter") in case
> len is 0
> (error or malformed tag), the stream position may be somewhere
> between ID3v2
> flags and ID3v2 size, but we step 4.

The 4 that are stepped here are "ID3" and the major version. At least one
more byte has been read after these by id3v2_tag_size(), so hdr has to be
fully refilled, hence step = 4. id3v2_tag_size() could be modified to fill
hdr before stepping, which would limit the number of dropped bytes, but
that would be ugly, and not very useful as Reimar said.

> If len > demuxer->movi_end - demuxer->movi_start, it cannot be
> decided
> whether this is a malformed footer or just audio data. There should
> be
> neither an error message nor a len setting in this case. (Same, if
> the
> corresponding and valid ID3 header cannot be found.)

No. In both cases, the ID3 footer signature is perfectly verified, so there
is no chance it can be audio data, and it has to be some leftover of an ID3
tag edition. Both cases are really errors because they prove that the stream
is malformed. IMO, it's better to drop the final MP3 frame than to play
garbage.

> Error (wouldn't warning or hint have been sufficient here?)

These are stream structure errors, so they should be reported with the message
level that MPlayer usually sets for such events. "Bad wav header length" and
"truncated extradata" are similar events that are also logged as errors, so
there is no reason to do something different for ID3 tags.

> messages,
> if any,
> should be translatable.

What does it imply for MPlayer? I don't see anything special done for the
two similar messages I mentioned above.

> I'm not sure whether it is worth the effort to check the whole 3DI
> footer
> data against the ID3 header data. According to the specs they must be
> copies.

They're supposed to be, but what if they're not? You have already made me
add a lot of checks for impossible cases if streams are spec-compliant.
We should either support only spec-compliant streams, or stick to 'work
around all possible garbage'.

> BTW, after applying I would change stream_seek(s,s->end_pos-128) to
> stream_seek(s,demuxer->movi_end-128) to avoid problems if the
> sequence of
> checks ever changes or a new check will be prepended.

Yes, and "demuxer->movi_end = stream_tell(s)-3;" could be replaced with
"demuxer->movi_end -= 128;".

Benoît
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 1-demux-audio-skip-id3v2.4.patch
Type: text/x-patch
Size: 3634 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20120118/3142d5d5/attachment.bin>