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

[Benoît Thébaudeau]

Hi Ingo, Reimar, all,

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

Ping?

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

Done in the extra patch attached.

Best regards,
Benoît
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 1-demux-audio-use-movi-end.patch
Type: text/x-patch
Size: 738 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20120127/566b59a5/attachment.bin>