[MPlayer-dev-eng] [PATCH] handle vobsub idx information from lavf demuxer

[Reimar Döffinger]

On Sat, Aug 30, 2008 at 02:31:22PM +0200, Aurelien Jacobs wrote:
> Reimar Döffinger wrote:
> > > +    do {
> > > +        if (!strncmp(ptr, "size:", 5))
> > > +            sscanf(ptr, "size: %dx%d", &sh->width, &sh->height);
> > 
> > Also, I have some doubts it is okay to assume that extradata is
> > 0-terminated.
> 
> It is not.
> That's why the loop check that ptr won't overflow extradata:
>     ptr < sh->extradata+sh->extradata_len
> Now if extradata is truncated in the middle of a line (ie. corrupted
> extradata), sscanf could read wrong values. But what would you expect
> from reading corrupted extradata ?

Well, not crashing if it can be avoided would be a good start. Picking
some sane defaults and doing the best to cover it up would be even
better.
The ptr < sh->extradata+sh->extradata_len check is basically done after
it is already too late...

> > Unfortunately changing it would be an API change, but am I the only one
> > who thinks this extradata format is not just a bit but actually
> > _extremely_ stupid?
> 
> What do you consider stupid here ? The lack of 0-termination or the
> text format I'm parsing here ?
> Adding a zero termination could be nice.
> About the text format parsed here, it is the format of .idx dvd files
> IIRC, and that's exactly what is stored in matroska files for example.
> And it have to be kept for remuxing, etc...

The information must be kept, but the text format certainly not! Sure,
you might loose the funny comments someone put in there (and which I
think will break your parsing code) but that is not what this is there
for.
Also, text parsing is _never_ trivial, and this approach means every
single application will duplicate this code. Or just forget to implement
it - which is the current situation even in FFmpeg (dvdsubdec.c does not
parse extradata).
I know from experience that some people disagree, but my opinion is that
if you have a design in which you might end up using sprintf and just a
few hundred lines of executed code later doing sscanf your design is
stupid - and at the latest when someone adds code to change the palette
in libavformat/libavcodec that is what has to be done with this design.
Probably this should be discussed in ffmpeg-dev though...

Greetings,
Reimar Döffinger