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

I have not received any answer for more than 2 weeks. Are you guys too busy, or
is there any problem?

I am new to this mailing list, so I don't know the average response time. Please
tell me if I'm just too eager.

Best regards,
Benoît