[MPlayer-dev-eng] vbr mp3 runtime

[Reimar Döffinger]

On Sun, Sep 25, 2011 at 07:00:55PM +0200, Ingo Brückl wrote:
> >> @@ -382,6 +440,8 @@
> >>       demux_info_add(demuxer,"Genre",genres[g]);
> >>        }
> >>      }
> >> +    if (duration) sh_audio->wf->nAvgBytesPerSec = demuxer->movi_end / duration;
> >> +    sh_audio->i_bps = sh_audio->wf->nAvgBytesPerSec;
> 
> > Why not just move this to where sh_audio->i_bps is already assigned
> > currently?
> 
> Because demuxer->movi_end is needed which is set correctly only after the
> tags evaluating code.
> 
> > Also I am not sure demuxer->movi_end is necessarily set/!= 0.
> 
> Well, if so, it's already with the current code. demux_audio.patch will
> ensure it is set, but that is not directly related to the vbr patch.

The point is that movi_end can be 0 (there are quite a few checks for
that in the code) and then you'll (maybe?) overwrite the bitrate to 0!
Otherwise go ahead, though I would have a few comments...

> +static unsigned int mp3_vbr_frames(stream_t *s, off_t off) {

Not much point yet, but in principle I'm in favour of completely
avoiding off_t, it tends to be 32 bit on MinGW, which means that
supporting large files without hacking the system headers is basically
not possible.
If we had used (u)int64_t everywhere it would just be a matter of using
the 64 bit functions in stream_file...

> +  unsigned int data;
> +  unsigned char hdr[4];

When you assume a certain number of bits, it's generally nicer
to use the uint32_t and uint8_t etc. types. It's also less to
write and doesn't give people really horrible ideas like just using
"char" when they need a signed type.

> +  const off_t xing_offt[2][2] = {{32, 17}, {17, 9}};

off_t is a bit overkill really.
Also adding "static" will avoid some compilers doing something stupid
like creating it on stack with every call.