[FFmpeg-devel] [PATCH] Indeo 5 decoder
Sat Feb 6 13:10:02 CET 2010
On Sat, Feb 06, 2010 at 11:07:06AM +0100, Reimar D?ffinger wrote:
> On Sat, Feb 06, 2010 at 11:39:24AM +0200, Kostya wrote:
> > On Sat, Feb 06, 2010 at 10:07:10AM +0100, Reimar D?ffinger wrote:
> > > On Thu, Feb 04, 2010 at 09:44:13AM +0200, Kostya wrote:
> > > > On Wed, Feb 03, 2010 at 09:27:29PM +0100, Reimar D?ffinger wrote:
> > > > > On Mon, Feb 01, 2010 at 07:49:55PM +0200, Kostya wrote:
> > > > > > + uint32_t frame_num;
> > > > >
> > > > > Only a 8-bit value is stored in that, also it is unused...
> > > >
> > > > still, it's in the bitstream. I'd like to keep it for debug purposes.
> > >
> > > I wasn't objecting to reading it, I just don't like it much that
> > > you're storing it in a context variable, that makes it rather hard
> > > to find out it's not used.
> > > Why do you think it is worse to do e.g.
> > >
> > > > + ctx->frame_num = get_bits(&ctx->gb, 8);
> > >
> > > skip_bits(&ctx->gb, 8); // frame_num
> > > (or keep the get_bits, then it's even less change to re-add it).
> > Not that I have any strict preferences, decoder author used this and I'd
> > like to keep it this way.
> Then at least add a comment somewhere for the poor person trying to find out
> what the purpose of that is.
> Possibly sort all those unused variables at the end and put a "// these are currently
> not used" above it or something.
> Also the "decoder author used this" would be more convincing if I knew if this was
> intentional or if it's just a left-over from the beginning when it was unclear
> if it would be needed or not.
Should be obvious for frame_num, can't say anything for the other fields,
but it looks to me that Maxim tried to follow original codec structure
Anyway, any other comments?
More information about the ffmpeg-devel