On mar, apr 08, 2014 at 10:01:56 +0200, Luca Barbato wrote: > On 08/04/14 17:16, Alessandro Ghedini wrote: > > From: Michael Niedermayer <[email protected]> > > > > Instead of using a fixed bitrate_idx, try to calculate a matching bitrate > > for > > the XING header. > > > > Using a fixed bitrate_idx causes tools such as file(1) and mediainfo(1) to > > report wrong bitrate and bitrate mode when using CBR. > > > > Bug: https://bugs.debian.org/736088 > > --- > > This was adapted from ffmpeg's commit 40176fc3 and pre-existing code. > > > > libavformat/mp3enc.c | 45 +++++++++++++++++++++++++++++++++++++++++---- > > 1 file changed, 41 insertions(+), 4 deletions(-) > > > > diff --git a/libavformat/mp3enc.c b/libavformat/mp3enc.c > > index 46889fc..4d9ba2b 100644 > > --- a/libavformat/mp3enc.c > > +++ b/libavformat/mp3enc.c > > @@ -119,6 +119,8 @@ static void mp3_write_xing(AVFormatContext *s) > > MPADecodeHeader mpah; > > int srate_idx, i, channels; > > int bitrate_idx; > > + int best_bitrate_idx; > > + int best_bitrate_error = INT_MAX; > > int xing_offset; > > int ver = 0; > > > > @@ -150,14 +152,49 @@ static void mp3_write_xing(AVFormatContext *s) > > return; > > } > > > > - /* 64 kbps frame, should be large enough */ > > - bitrate_idx = (ver == 3) ? 5 : 8; > > - > > /* dummy MPEG audio header */ > > header = 0xff << 24; // sync > > header |= (0x7 << 5 | ver << 3 | 0x1 << 1 | 0x1) << 16; // > > sync/audio-version/layer 3/no crc*/ > > - header |= (bitrate_idx << 4 | srate_idx << 2) << 8; > > + header |= (srate_idx << 2) << 8; > > header |= channels << 6; > > + > > + for (bitrate_idx = 1; bitrate_idx < 15; bitrate_idx++) { > > + int error; > > + > > So you build the header, > > > + avpriv_mpegaudio_decode_header(&mpah, header | (bitrate_idx << > > (4+8))); > > And then parse it again instead of > > getting > > layer = 4 - ((header >> 17) & 3);
Isn't layer always 3 here?
> if (header & (1<<20) && header & (1<<19))
> lsf = 0;
> else
> lsf = 1;
>
> once
>
> and then compute bitrate as
>
> bit_rate = 1000 * avpriv_mpa_bitrate_tab[lsf][layer - 1][bitrate_index];
>
> and be done with that.
Right. I guess avpriv_mpegaudio_decode_header() was used to avoid duplicating
code though, but since it's just a few lines it doesn't look bad
> > + error = FFABS(mpah.bit_rate - codec->bit_rate);
> > +
> > + if (error < best_bitrate_error){
> > + best_bitrate_error = error;
> > + best_bitrate_idx = bitrate_idx;
> > + }
> > + }
> > + av_assert0(best_bitrate_idx >= 0);
>
> Please do not bring asserts when porting patches.
Ok.
> > + for (bitrate_idx = best_bitrate_idx; bitrate_idx < 15; bitrate_idx++) {
> > + int needed;
> > + int32_t mask;
> > +
> > + mask = bitrate_idx << (4 + 8);
> > + header |= mask;
> > + avpriv_mpegaudio_decode_header(&mpah, header);
>
> I think the number of channels is already available above w/out having
> to parse again many times the header.
Yeah, but it later needs mpah.frame_size...
> > + xing_offset = xing_offtbl[mpah.lsf == 1][mpah.nb_channels == 1];
> > + needed = 4 // header
> > + + xing_offset
> > + + 4 // xing tag
> > + + 4 // frames/size/toc flags
> > + + 4 // frames
> > + + 4 // size
> > + + XING_TOC_SIZE // toc
> > + + 24
> > + ;
> > +
> > + if (needed <= mpah.frame_size)
> > + break;
> > +
> > + header &= ~mask;
> > + }
>
> > avpriv_mpegaudio_decode_header(&mpah, header);
>
> Maybe I can get you a function returning bitrate and framesize and make
> this code a bit more simple.
...that would help, if anything to avoid the avpriv_mpegaudio_decode_header()
calls.
There's also more room for clean-ups (e.g. xing_offset is calculated inside the
loop and after, the "needed" calculation is also duplicated, ...), though since
I'm not very familiar with the codebase I was afraid of breaking something (I'll
include these in the next version, since it seems to work fine).
Cheers
--
perl -E '$_=q;$/= @{[@_]};and s;\S+;<inidehG ordnasselA>;eg;say~~reverse'
signature.asc
Description: Digital signature
_______________________________________________ libav-devel mailing list [email protected] https://lists.libav.org/mailman/listinfo/libav-devel
