[FFmpeg-devel] [PATCH] rmdec: avoid leaving st unitialized

[Reimar Döffinger]

On Sun, Jun 06, 2010 at 03:33:03PM -0400, Ronald S. Bultje wrote:
> On Sun, Jun 6, 2010 at 2:45 PM, Reimar D?ffinger
> <Reimar.Doeffinger at gmx.de> wrote:
> > On Sun, Jun 06, 2010 at 01:03:20PM -0400, Ronald S. Bultje wrote:
> >> On Sun, Jun 6, 2010 at 6:57 AM, Reimar D?ffinger
> >> <Reimar.Doeffinger at gmx.de> wrote:
> >> > this fixes a warning with my gcc version, and I think it even fixes a bug:
> >> > Index: rmdec.c
> >> > ===================================================================
> >> > --- rmdec.c ? ? (revision 23501)
> >> > +++ rmdec.c ? ? (working copy)
> >> > @@ -843,7 +844,7 @@
> >> > ? ? ? ? ? ? ? ? pos = url_ftell(s->pb);
> >> > ? ? ? ? ? ? } else {
> >> > ? ? ? ? ? ? ? ? len=sync(s, &timestamp, &flags, &i, &pos);
> >> > - ? ? ? ? ? ? ? ?if (len > 0)
> >> > + ? ? ? ? ? ? ? ?if (len >= 0)
> >> > ? ? ? ? ? ? ? ? ? ? st = s->streams[i];
> >> > ? ? ? ? ? ? }
> >> >
> >> >
> >> > The next line below this reads:
> >> > ? ? ? ? ? ?if(len<0 || url_feof(s->pb))
> >> > ? ? ? ? ? ? ? ?return AVERROR(EIO);
> >> > so >= 0 fits with this condition, and other places treat a 0 value
> >> > from "sync" as ok.
> >>
> >> sync() can not return zero (I've looked at this a couple of times in
> >> the past because the gcc warning annoyed me at some point). So the
> >> patch silences a warning but in reality, it doesn't do anything.
> >
> > ok, no bug, but that still leaves a compiler warning and that there's
> > no point to treat 0 once this way once the other way...
> > And if you "looked at this a couple of times" there's also obviously
> > the point that it obviously wastes developer time the way it is now.
> 
> It's like tossing a coin, shall we treat 0 as error or as success?

For other container formats, 0 can be a valid packet size.
And for audio packets, it seems this values is never used (and for
video it is not really checked much either).

> :-). I'd say error, but please then document that in reality this will
> never happen and that it's just there to silence a compiler warning
> (or so). OK with you Kostya?

Well, a doxy for sync() would be the best... (wouldn't hut for some other
functions).
Any volunteer to go through it and add a head dump of what they remember?
I'd then volunteer to improve or clarify it if necessary...