[FFmpeg-cvslog] r12144 - trunk/libavcodec/mpeg12.c

[Rich Felker]

On Tue, Feb 19, 2008 at 09:24:30AM -0800, Mike Melanson wrote:
> Rich Felker wrote:
> > On Tue, Feb 19, 2008 at 04:55:14PM +0100, michael wrote:
> >> Author: michael
> >> Date: Tue Feb 19 16:55:14 2008
> >> New Revision: 12144
> >>
> >> Log:
> >> Reduce the number of senselessly scanned bytes.
> >>
> >>
> >> Modified:
> >>    trunk/libavcodec/mpeg12.c
> >>
> >> Modified: trunk/libavcodec/mpeg12.c
> >> ==============================================================================
> >> --- trunk/libavcodec/mpeg12.c	(original)
> >> +++ trunk/libavcodec/mpeg12.c	Tue Feb 19 16:55:14 2008
> >> @@ -1869,7 +1869,7 @@ static int mpeg_decode_slice(Mpeg1Contex
> >>          }
> >>      }
> >>  eos: // end of slice
> >> -    *buf += get_bits_count(&s->gb)/8 - 1;
> >> +    *buf += (get_bits_count(&s->gb)-1)/8;
> > 
> > Have you checked what code gcc generates for this /8?? I suspect it's
> > a division!! Use >>3 or fix get_bits_count to return unsigned instead
> > of int.
> 
> gcc 4.1.3 on Ubuntu/x86_32 compiles the operation down to a sar
> instruction. Though I agree that it's a good idea to specify bit shifts
> in the interest of bit accuracy, at least for these applications.

I don't see how it's an issue of bit accuracy here. Rather it's an
issue of the compiler not necessarily knowing that it's safe to use a
bit shift due to signedness issues. Also for instance, if
get_bits_count can return 0, then using a shift is not safe unless
it's a signed shift.

Rich