[PATCH] Add support for VBR files without Xing headers
Jeff Mitchell
kde-dev at emailgoeshere.com
Mon Jan 22 21:13:17 CET 2007
Xavier--
If I'm reading this right, you iterate through the entire file, parsing every
header along the way.
Please revert this patch until a consensus can be made, including Scott's
opinion, as to whether this is a good thing. Scanning the entire file to
calculate the length will severely slow down applications that use taglib for
scanning large amounts of files where users have large numbers of these
broken VBR files -- every time the file is scanned, to boot.
Please see my earlier email to taglib-devel here:
http://www.nabble.com/First-results-of-my-evaluation-of-taglib-against-id3tag-t2947115.html
as well as the bug report mentioned for reasons why hard-coding this into
taglib is not necessarily a good idea.
If you want to have this option available, I think a better approach would be
to make it just that -- an option. Perhaps if the file is a broken VBR file,
you could set a boolean that could be checked in client code -- something
like bool brokenVBR(). Then the client code would have the option of running
this calculation code, with an option of then writing the appropriate Xing
header: int correctVBRLength( bool writeXing ).
I think that would be a much more useful approach, with the advantage that
future rescans of the file would be much faster if the option to write the
Xing header is selected. And it avoids forcing extremely lengthy full-file
scans on clients that don't want them.
--Jeff
On Monday 22 January 2007 10:47, Xavier Duret wrote:
> This patch adds support for VBR files without Xing headers. I have a
> few files for which this patch does not work because the first MPEG
> frame is not where it is supposed to be. The patch has to be applied
> after the set of patches that I sent previously. List of changes:
> - Fix a couple of typos.
> - Fix the calculation of the frame length.
> - Tidy up nextFrameOffset method.
> - Add support for VBR files without Xing headers.
>
> diff -ruN taglib.old/taglib/mpeg/mpegheader.cpp
> taglib/taglib/mpeg/mpegheader.cpp
> --- taglib.old/taglib/mpeg/mpegheader.cpp 2007-01-17
> 16:01:31.000000000 +0100
> +++ taglib/taglib/mpeg/mpegheader.cpp 2007-01-22 16:30:17.000000000 +0100
> @@ -153,7 +153,7 @@
> void MPEG::Header::parse(const ByteVector &data)
> {
> if(data.size() < 4 || uchar(data[0]) != 0xff) {
> - debug("MPEG::Header::parse() -- First byte did not mactch MPEG
> synch."); + debug("MPEG::Header::parse() -- First byte did not match
> MPEG synch."); return;
> }
>
> @@ -162,7 +162,7 @@
> // Check for the second byte's part of the MPEG synch
>
> if(!flags[23] || !flags[22] || !flags[21]) {
> - debug("MPEG::Header::parse() -- Second byte did not mactch MPEG
> synch."); + debug("MPEG::Header::parse() -- Second byte did not match
> MPEG synch."); return;
> }
>
> @@ -242,11 +242,10 @@
> d->isPadded = flags[9];
>
> // Calculate the frame length
> -
> if(d->layer == 1)
> - d->frameLength = 24000 * 2 * d->bitrate / d->sampleRate +
> int(d->isPadded); + d->frameLength = (12 * d->bitrate * 1000 /
> d->sampleRate +
> int(d->isPadded)) * 4;
> else
> - d->frameLength = 72000 * d->bitrate / d->sampleRate +
> int(d->isPadded); + d->frameLength = 144 * d->bitrate * 1000 /
> d->sampleRate +
> int(d->isPadded);
>
> // Now that we're done parsing, set this to be a valid frame.
>
> diff -ruN taglib.old/taglib/mpeg/mpegfile.cpp
> taglib/taglib/mpeg/mpegfile.cpp --- taglib.old/taglib/mpeg/mpegfile.cpp
> 2007-01-22 17:44:04.000000000 +0100 +++ taglib/taglib/mpeg/mpegfile.cpp
> 2007-01-22 17:44:21.000000000 +0100 @@ -447,7 +447,7 @@
> {
> // TODO: This will miss syncs spanning buffer read boundaries.
>
> - ByteVector buffer = readBlock(bufferSize());
> + ByteVector buffer(bufferSize(),0);
>
> while(buffer.size() > 0) {
> seek(position);
> diff -ruN taglib.old/taglib/mpeg/mpegproperties.cpp
> taglib/taglib/mpeg/mpegproperties.cpp
> --- taglib.old/taglib/mpeg/mpegproperties.cpp 2007-01-22
> 17:56:06.000000000 +0100
> +++ taglib/taglib/mpeg/mpegproperties.cpp 2007-01-22
> 17:12:59.000000000 +0100
> @@ -225,13 +225,48 @@
>
> // TODO: Make this more robust with audio property detection for
> VBR without a
> // Xing header.
> + long position = first;
> + uint nSamples = 0;
> + uint nFrames = 0;
> + int bitRate = 0;
> + bool cbr = true;
> + d->file->seek(position);
> + while (position < d->file->length()) {
> + Header currentHeader(d->file->readBlock(4));
> + if (currentHeader.isValid()) {
> + position += currentHeader.frameLength();
> + nSamples += (currentHeader.layer() > 1) ? 1152 : 384;
> + d->file->seek(position);
> +
> + // Assume that if a file has a constant bit rate for more
> than 100 frames
> + // then it is CBR and there is no point in parsing it complely.
> + if (bitRate == 0)
> + bitRate = currentHeader.bitrate();
> + else {
> + if (cbr && (bitRate != currentHeader.bitrate())) {
> + cbr = false;
> + bitRate = currentHeader.bitrate();
> + }
> + }
> + nFrames += 1;
> + if (cbr && nFrames > 100)
> + break;
>
> - if(firstHeader.frameLength() > 0 && firstHeader.bitrate() > 0) {
> - int frames = (last - first) / firstHeader.frameLength() + 1;
> + } else
> + break;
> + }
>
> - d->length = int(float(firstHeader.frameLength() * frames) /
> - float(firstHeader.bitrate() * 125) + 0.5);
> - d->bitrate = firstHeader.bitrate();
> + if (position >= d->file->length()) {
> + d->length = nSamples / firstHeader.sampleRate();
> + d->bitrate = d->length > 0 ? (position - first) / d->length / 1000 :
> 0; + } else {
> + if(firstHeader.frameLength() > 0 && firstHeader.bitrate() > 0) {
> + int frames = (last - first) / firstHeader.frameLength() + 1;
> +
> + d->length = int(float(firstHeader.frameLength() * frames) /
> + float(firstHeader.bitrate() * 125) + 0.5);
> + d->bitrate = firstHeader.bitrate();
> + }
> }
> }
> _______________________________________________
> taglib-devel mailing list
> taglib-devel at kde.org
> https://mail.kde.org/mailman/listinfo/taglib-devel
More information about the taglib-devel
mailing list