On sab, apr 12, 2014 at 01:53:50 +0200, Anton Khirnov wrote: > > On Sat, 12 Apr 2014 13:23:52 +0200, Alessandro Ghedini > <[email protected]> wrote: > > On sab, apr 12, 2014 at 09:04:56 +0200, Anton Khirnov wrote: > > > > > > On Wed, 9 Apr 2014 18:50:05 +0200, Alessandro Ghedini > > > <[email protected]> wrote: > > > > --- > > > > libavformat/mp3dec.c | 57 > > > > ++++++++++++++++++++++++++++++++++++++++++++++++++++ > > > > 1 file changed, 57 insertions(+) > > > > > > > > diff --git a/libavformat/mp3dec.c b/libavformat/mp3dec.c > > > > index 1ab5159..e33f524 100644 > > > > --- a/libavformat/mp3dec.c > > > > +++ b/libavformat/mp3dec.c > > > > @@ -132,7 +132,17 @@ static void read_xing_toc(AVFormatContext *s, > > > > int64_t filesize, int64_t duration > > > > static void mp3_parse_info_tag(AVFormatContext *s, AVStream *st, > > > > MPADecodeHeader *c, uint32_t spf) > > > > { > > > > +#define LAST_BITS(k, n) ((k) & ((1 << (n)) - 1)) > > > > +#define MIDDLE_BITS(k, m, n) LAST_BITS((k) >> (m), ((n) - (m))) > > > > + > > > > + float f; > > > > uint32_t v; > > > > + > > > > + char version[10]; > > > > + > > > > + uint32_t peak = 0; > > > > + int32_t r_gain = INT32_MIN, a_gain = INT32_MIN; > > > > + > > > > MP3DecContext *mp3 = s->priv_data; > > > > const int64_t xing_offtbl[2][2] = {{32, 17}, {17,9}}; > > > > > > > > @@ -152,6 +162,53 @@ static void mp3_parse_info_tag(AVFormatContext *s, > > > > AVStream *st, > > > > (AVRational){spf, > > > > c->sample_rate}, > > > > st->time_base)); > > > > } > > > > + > > > > + avio_skip(s->pb, 3); > > > > + > > > > + /* VBR quality */ > > > > + avio_r8(s->pb); > > > > > > Doesn't matter much since we ignore the value, but isn't vbr quality this > > > whole > > > 32bit number? > > > > http://gabriel.mp3-tech.org/mp3infotag.html says it's only one byte. > > > > But at the top of the page it has > // 4 bytes for VBR SCALE. a VBR quality indicator: 0=best 100=worst
Uh, weird. > Anyway, doesn't matter. Keep it as you like. Well, using rb32() lets us merge the avio_skip() with the read, so it's probably better your way. > > > > + > > > > + /* Encoder short version string */ > > > > + avio_get_str(s->pb, 9, version, 10); > > > ^^ > > > sizeof(version) please > > > > > > Also, perhaps we should export this as 'encoder' in the stream metadata > > > (not in > > > global, so we don't conflict with whatever is read from the ID3 tag). > > > > This doesn't seem to work. I added: > > > > av_dict_set(&st->metadata, "encoder", version, 0); > > > > But when I use avprobe on a file encoded with libav, "encoder" is an empty > > string instead of "Lavf..." (i.e. it does conflict with the id3 tag). > > Our lame wrapper disables the LAME tag, so that is quite expected. > Try it on some file that is generated by lame directly. Yeah, nevermind. I misunderstood what you meant initially. > > > And I wonder if it's safe to unconditionally assume the tag is in the LAME > > > format. IIUC, it can be just the XING tag with garbage/zeroes filling the > > > rest > > > of the frame? > > > > > > Is there some way to check that the tag is really in the LAME format? > > > > The replaygain values already have a check, for the encoder string, maybe > > checking that version[0] != '\0', and that all the characters are >= 32. > > > > Another way would be to check the header CRC16 (which covers the first 190 > > bytes > > of the header) before exporting any information (encoder and replaygain). > > Not > > sure how reliable that is though. > > That sounds like a good idea to me. Ok, so, LAME seems to be using a reversed ANSI CRC16, which is missing from libav. I'll make a patch to add that too. > > Btw, I just realized that the function doesn't return if the MKBETAG() > > checks > > fail (the fix will be in the next version of the patch). > > > > > > + > > > > + /* Info Tag revision + VBR method */ > > > > + avio_r8(s->pb); > > > > + > > > > + /* Lowpass filter value */ > > > > + avio_r8(s->pb); > > > > + > > > > + /* ReplayGain peak */ > > > > + v = avio_rb32(s->pb); > > > > + f = (v << 5) / (float) (1 << 28); > > > > > > http://gabriel.mp3-tech.org/mp3infotag.html says this value is stored as > > > float > > > (nb: people doing such stuff need to be stabbed with a rusty spork > > > repeatedly), > > > so perhaps use av_int2float()? > > > > That doesn't work (i.e. it always return 0.0). I copied the decoding formula > > from eyed3 [0] so I'm not quite sure if that's correct, but it does seem to > > return realistic peak values. > > Looking at LAME itself, seems it takes the float peak value (with 1.0 being > full > scale), multiplies it by 1<<23, casts the result to int, and writes that. > > So something like > av_rescale(v, 100000, 1 << 23) > should work. Yep, that works. -- perl -E '$_=q;$/= @{[@_]};and s;\S+;<inidehG ordnasselA>;eg;say~~reverse' _______________________________________________ libav-devel mailing list [email protected] https://lists.libav.org/mailman/listinfo/libav-devel
