[Music] Incorrect track length; Support variable-bitrate MP3s

RESOLVED FIXED

Status

Firefox OS
Gaia::Music
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: tzimmermann, Assigned: tzimmermann)

Tracking

unspecified
ARM
Gonk (Firefox OS)
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(b2g18+)

Details

Attachments

(8 attachments, 11 obsolete attachments)

9.52 KB, patch
tzimmermann
: review+
Details | Diff | Splinter Review
17.36 KB, patch
tzimmermann
: review+
Details | Diff | Splinter Review
6.61 KB, patch
tzimmermann
: review+
Details | Diff | Splinter Review
11.86 KB, patch
tzimmermann
: review+
Details | Diff | Splinter Review
9.02 KB, patch
tzimmermann
: review+
Details | Diff | Splinter Review
378.46 KB, patch
padenot
: review+
Details | Diff | Splinter Review
257.67 KB, image/png
Details
831 bytes, patch
padenot
: review+
Details | Diff | Splinter Review
I have an mp3 album for which the track numbers are almost always incorrect. The songs are all 3 to 4 minutes log, but for most the duration is either too long (~20 min) or too short. In the latter case, the music player stops playing the track when the displayed time has been reached and continues with the subsequent track.

Version: e9dfbe2e99bfec5c1609b8e7fafe54477914c715 from git://github.com/mozilla-b2g/B2G.git (b2g18)
Gecko: b75dfee39f8a5b634a9bc39dacf2bdf59ee4333f
Gaia: df38c1bb813029f3ccfa4a997fb1529b3ff1a1ff
tracking-b2g18: --- → ?

Comment 1

6 years ago
Thomas, can you provide some link or attach your music file? thanks.
Created attachment 703338 [details] [diff] [review]
Support variable-bitrate MP3 files

Even better, I have a patch for the bug. :)

An MP3 is split into small frames of 26ms. The problem is that the files use variable bitrates (i.e., each MP3 frame can have an individual bitrate). Android only looks at the first frame when computing the duration. With the patch applied, all frames are considered, which results in the correct duration. The patch is for Android code in frameworks/base/, I don't know how we ship such changes; if at all.

Also, we now need to download all of an MP3 file to, which makes streaming impossible. Maybe we should patch the music-player app instead to not stop playing when there is still data available. Not sure if that is possible, though.
Attachment #703338 - Flags: feedback?(dkuo)
Summary: [Music] Incorrect track length → [Music] Incorrect track length; Support variable-bitrate MP3s

Comment 3

6 years ago
Comment on attachment 703338 [details] [diff] [review]
Support variable-bitrate MP3 files

Good to know you already have a patch for this.

Looks like this issue is caused by VBR mp3, so I think the media platform guys might be better to give some feedback to your patch. Reassigning to Chris Double, thanks.
Attachment #703338 - Flags: feedback?(dkuo) → feedback?(chris.double)

Comment 4

6 years ago
Comment on attachment 703338 [details] [diff] [review]
Support variable-bitrate MP3 files

Review of attachment 703338 [details] [diff] [review]:
-----------------------------------------------------------------

::: media/libstagefright/MP3Extractor.cpp
@@ +344,5 @@
> +
> +              off += frame_size;
> +
> +              // each frame contains 26ms, resp. 26000us
> +              durationUs += 26000;

This number should be computed from sampling rate and samples per frame of a frame.  you can get sampling rate from corresponding frame header.  http://www.codeproject.com/Articles/8295/MPEG-Audio-Frame-Header

Comment 5

6 years ago
Comment on attachment 703338 [details] [diff] [review]
Support variable-bitrate MP3 files

Does this patch mean that B2G would need to download the entirety of an MP3 before it started playing? If so this would make this approach impractical.
Attachment #703338 - Flags: feedback?(chris.double) → feedback-
(In reply to Chris Double (:doublec) from comment #5)
> Comment on attachment 703338 [details] [diff] [review]
> Support variable-bitrate MP3 files
> 
> Does this patch mean that B2G would need to download the entirety of an MP3
> before it started playing? If so this would make this approach impractical.

I think so. To calculate the total duration, we need to walk over the whole file and look at each individual frame. And each frame is just a few 100 B in size.

I thought of this patch more of a prove-of-concept than the final solution. Maybe we could distinguish between local files and remote files, and only apply correct calculation in the former case. We should probably also change the music app or the underlying C code to not stop when the calculated duration has been reached. That's what my Linux music player does, btw. It also computes an incorrect duration initially, but continues playing until the end of the file has been reached.
How about we re-evaluate the duration every now and then? We'd start with the duration coming from Android, but when we have more data available, we re-read the data and update the duration. For constant-bitrate MP3s, nothing would change, and for variable-bitrate MP3s, we'd adapt to the real length while we're playing.

As a side effect, fixing this bug might also fix bug 783512, where we work around ID3 tags at the end of the file.
Assignee: nobody → tzimmermann
If I remove lines 645 to 659 from apps/music/js/Player.js, the MP3s play to the end, although with a wrong duration shown. It's the workaround for bug 783512 that makes the music player skip parts of the tracks.
Created attachment 704875 [details] [diff] [review]
Decode MP3 frame headers

With this patch, Gecko walks over each received fragment of MP3 data, decodes the frame headers, and updates the duration accordingly. This allows to calculate the correct duration for an MP3 file and support streaming at the same time. Other file types are ignored. In future patches, we could add support for XING and VBRI headers, which might be embedded within the MP3 file.

This patch also seems to fix bug 783512. At least I removed the respective workaround from Gaia's music-player app, but the player still advances from one title to the next. I also tested this with the MP3 file that is attached to bug 783512.
Attachment #703338 - Attachment is obsolete: true
Attachment #704875 - Flags: review?(chris.double)
Status: NEW → ASSIGNED
Created attachment 706322 [details] [diff] [review]
Decode MP3 frame headers [v2]

With version two of this patch the MP3 duration gets updated by running MediaReader::NotifyDataArrived, instead of the implicit update from within OmxDecoder::NotifyDataArrived.
Attachment #704875 - Attachment is obsolete: true
Attachment #704875 - Flags: review?(chris.double)
Attachment #706322 - Flags: feedback?(chris.double)
tracking-b2g18: ? → +
Hi Chris, could you give me some feedback on the attached patch or forward the request to someone else. Thanks!
Flags: needinfo?(chris.double)

Comment 12

5 years ago
Comment on attachment 706322 [details] [diff] [review]
Decode MP3 frame headers [v2]

This is not my field of expertise, I suggest finding someone else for feedback.
Attachment #706322 - Flags: feedback?(chris.double)
Flags: needinfo?(chris.double)
Paul could you give a feedback to Thomas for this patch please ?
Flags: needinfo?(paul)
Comment on attachment 706322 [details] [diff] [review]
Decode MP3 frame headers [v2]

Review of attachment 706322 [details] [diff] [review]:
-----------------------------------------------------------------

This will indeed solve a couple issues (I can think of bug 832147 in addition to the bug you mentioned), but needs some more work.

::: content/media/omx/OmxDecoder.cpp
@@ +435,5 @@
> +  static const uint16_t sSampleRate[4] = {
> +    44100, 48000, 32000, 0
> +  };
> +
> +  unsigned long header = (aData[0]<<24) | (aData[1]<<16) |

Can you please only use "stdint.h" style integer types in this patch instead of the plain C types? I find those harder to reason with when doing bitwise work, and it fits the surrounding style better.

@@ +500,5 @@
> +
> +  if (!mMP3Offset) {
> +
> +    unsigned char data[10];
> +    nsresult rv = mResource->ReadFromCache(reinterpret_cast<char*>(data),

This does main thread IO and is not great for performances. I would use an approach close to what we do for WebM: do the processing solely on |aBuffer| without hitting the disk. It will certainly be more complex, but well, adding main thread IO is a no-go, especially because we are talking about flash drives with a super low throughput.
Flags: needinfo?(paul)
Attachment #706322 - Flags: feedback-

Updated

5 years ago
Blocks: 832147
Thanks for the feedback. I'll have a look at the WebM implementation and see if I can get the IO out of the main thread.
There are also extension headers for MP3 that add some information about track length, etc. I plan to add support for those as well, Once the support for plain-MP3 is ready. See http://www.codeproject.com/Articles/8295/MPEG-Audio-Frame-Header#VBRHeaders
Created attachment 745049 [details] [diff] [review]
[00] Remove trailing whitespaces
Attachment #745049 - Flags: review?(paul)
Created attachment 745050 [details] [diff] [review]
[01] Bug 831224: Added MP3 frame parser
Attachment #745050 - Flags: review?(paul)
Created attachment 745051 [details] [diff] [review]
[02] Bug 831224: Allow update of the media duration
Attachment #745051 - Flags: review?(paul)
Created attachment 745052 [details] [diff] [review]
[03] Bug 831224: Parse MP3 frame headers in OmxDecoder
Attachment #706322 - Attachment is obsolete: true
Attachment #745052 - Flags: review?(paul)
Attachment #745052 - Attachment description: [02] Bug 831224: Parse MP3 frame headers in OmxDecoder → [03] Bug 831224: Parse MP3 frame headers in OmxDecoder
Hi Paul!

This new patch set adds a dedicated parser for the MP3 frames. Parsing is always done in the main thread. Files are read once when the decode thread starts and are pushed to the main thread for parsing. This avoids main-thread I/O, and handles concurrency between decode thread and main thread.

I also have a patch for bug 783512 that removes the current workaround from the music player. I'll submit it when these patches here went in.

Updated

5 years ago
Attachment #745049 - Flags: review?(paul) → review+
BTW, I tested this with local MP3 files in the music app, and streamed MP3 audio in the browser app. The duration for the local files is always correct. The streams start with the duration of the available data, and update when new data arrives.

There is some overhead for reading in the local files when the decoder starts, but in practice no delay was noticeable to me. If we have MP3 files where this becomes a problem (maybe >100 MiB) we can always split this step into multiple read operations.
Comment on attachment 745050 [details] [diff] [review]
[01] Bug 831224: Added MP3 frame parser

Review of attachment 745050 [details] [diff] [review]:
-----------------------------------------------------------------

In general, this looks great, but we could make it a bit more maintainable/readable.

Could you add a comment at the top of this file with the documentation you used to write this, so someone modifying this file uses the same source and can understand your choices?

Also, could you put spaces around your operators? This is generally preferred in Gecko.

I'd like to have a final look with those comments addressed, so I can double check myself on the array-bounds and other risky stuff.

::: content/media/omx/MP3FrameParser.cpp
@@ +20,5 @@
> +  enum {
> +    ID3_HEADER_LENGTH = 10
> +  };
> +
> +  ID3Buffer(const unsigned char* aBuffer, uint32_t aLength)

uint8_t

@@ +35,5 @@
> +    return ID3_HEADER_LENGTH + mSize;
> +  }
> +
> +private:
> +  const unsigned char* mBuffer;

uint8_t

@@ +74,5 @@
> +  enum {
> +    MP3_HEADER_LENGTH = 4
> +  };
> +
> +  MP3Buffer(const unsigned char* aBuffer, uint32_t aLength)

uint8_t

@@ +86,5 @@
> +  {
> +    MOZ_ASSERT(mBuffer || !mLength);
> +  }
> +
> +  static const unsigned char* FindNextHeader(const unsigned char* aBuffer, uint32_t aLength);

uint8_t

@@ +111,5 @@
> +    return mTrailing;
> +  }
> +
> +private:
> +  static uint32_t ExtractFrameHeader(const unsigned char* aBuffer);

uint8_t and two other below.

@@ +147,5 @@
> +const uint16_t MP3Buffer::sSampleRate[4] = {
> +  44100, 48000, 32000, 0
> +};
> +
> +uint32_t MP3Buffer::ExtractFrameHeader(const unsigned char* aBuffer)

uint8_t

@@ +174,5 @@
> +         (version == 0x3) *
> +         (layer == 0x01) * !!bitRate * !!sampleRate * header;
> +}
> +
> +const unsigned char* MP3Buffer::FindNextHeader(const unsigned char* aBuffer, uint32_t aLength)

uint8_t * 2

@@ +192,5 @@
> +
> +  return aBuffer;
> +}
> +
> +nsresult MP3Buffer::DecodeFrameHeader(const unsigned char* aBuffer,

uint8_t

@@ +203,5 @@
> +  if (!header) {
> +    return NS_ERROR_INVALID_ARG;
> +  }
> +
> +  uint32_t bitRate = sBitRate[(header>>12)&0xf];

I feel like there is a big amount of magic number or bit shifting magic in this file. Maybe it would be good to factor common patterns such as this on in a macro or inline function, so it does not get out of sync between different uses for some reason.

@@ +207,5 @@
> +  uint32_t bitRate = sBitRate[(header>>12)&0xf];
> +  uint32_t sampleRate = sSampleRate[(header>>10)&0x3];
> +
> +  uint32_t padding = (header>>9)&0x1;
> +  uint32_t frameSize = (144000ul * bitRate) / sampleRate + padding;

Turn the 144000ul into a constant, please.

@@ +209,5 @@
> +
> +  uint32_t padding = (header>>9)&0x1;
> +  uint32_t frameSize = (144000ul * bitRate) / sampleRate + padding;
> +
> +  if (aBitRate) {

Maybe you could drop this check (and the two below), the only call site makes it kind of clear this will always be valid.

Just MOZ_ASSERT(aBitRate && aFrameSize && aDuration) it if you want to enforce this.

@@ +218,5 @@
> +    *aFrameSize = frameSize;
> +  }
> +
> +  if (aDuration) {
> +    *aDuration = (8000ul * frameSize) / bitRate;

Turn the 8000ul in a constant.

@@ +230,5 @@
> +  // We walk over the newly arrived data and sum up the
> +  // bit rates, sizes, durations, etc. of the contained
> +  // MP3 frames.
> +
> +  const unsigned char* buffer = mBuffer;

uint8_t

@@ +274,5 @@
> +  mTrailing(0),
> +  mIsMP3(true)
> +{ }
> +
> +void MP3FrameParser::Parse(const unsigned char* aBuffer, uint32_t aLength, int64_t aOffset)

I think this function could benefit from being split into several functions with meaningful names. Also, uin8t_t.

@@ +288,5 @@
> +      // We have some data in our temporary buffer and append to it, or
> +      // we are at the beginning of the stream. We both cases, we append
> +      // some data to out temporary buffer and try to parse it.
> +
> +      copyLength = std::min<size_t>(sizeof(mBuffer)-mBufferLength, aLength);

I'd prefer if NS_ARRAY_LENGTH was used here, it makes it clear |mBuffer| is a static array. 

[1]: http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsMemory.h#123

@@ +360,5 @@
> +    // Fragment comes after current position, store difference.
> +    mUnhandled += aOffset-mOffset;
> +
> +    // We might start in the middle of a frame and have find the next
> +    // frame header. As out detection heuristics might return false

s/out/our/

@@ +362,5 @@
> +
> +    // We might start in the middle of a frame and have find the next
> +    // frame header. As out detection heuristics might return false
> +    // positives, we simply try multiple times.
> +    tryFind = 5;

Can you add a comment to say the rationale behind choosing 5? If it is empiric, say it.

@@ +380,5 @@
> +      }
> +      --tryFind;
> +
> +      // We might be in the middle of a frame, find next frame header
> +      const unsigned char *buffer = MP3Buffer::FindNextHeader(aBuffer+1, aLength-1);

uint8_t

@@ +396,5 @@
> +    mNumFrames  += mp3Buffer.GetNumberOfFrames();
> +    mOffset     += mp3Buffer.GetFrameSizeSum();
> +
> +    trailing = mp3Buffer.GetTrailing();
> +  } while (false);

I find the logic of this block quite weird. How about something like this:

> while (tryFind) {
>   rv = mp3Buffer.Parse();
>   if (NS_FAILED(rv)) {
>     (...)
>     tryFind--;
>   } else {
>     (...)
>     tryFind = 0;
>   }
> }

you would have to slightly change the code above (the tryFind initialization).

@@ +400,5 @@
> +  } while (false);
> +
> +  if (trailing) {
> +    // Store trailing bytes in temporary buffer.
> +    memcpy(mBuffer, aBuffer+(aLength-trailing), trailing);

MOZ_ASSERT(trailing <= NS_ARRAY_LENGTH(mBuffer));

@@ +407,5 @@
> +}
> +
> +void MP3FrameParser::NotifyDataArrived(const char* aBuffer, uint32_t aLength, int64_t aOffset)
> +{
> +  Parse(reinterpret_cast<const unsigned char*>(aBuffer), aLength, aOffset);

uint8_t

@@ +412,5 @@
> +}
> +
> +int64_t MP3FrameParser::GetDuration()
> +{
> +  MutexAutoLock lockGuard(mLock);

nit: we tend to call those |mon| in the media code, and consistency is good :-)

There are more than one occurrence of this.

::: content/media/omx/MP3FrameParser.h
@@ +4,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include <stdint.h>
> +#include "mozilla/Mutex.h"

#include <mozilla/StandardInteger.h>, for reasons explained in the file.

@@ +25,5 @@
> +    MutexAutoLock lockGuard(mLock);
> +    return mIsMP3;
> +  }
> +
> +  void Parse(const unsigned char* aBuffer, uint32_t aLength, int64_t aOffset);

uint8_t, and below as well.

@@ +32,5 @@
> +
> +  int64_t GetDuration();
> +
> +private:
> +  unsigned char mBuffer[10];

Prefer stdint.h-like types.
Attachment #745050 - Flags: review?(paul)
Comment on attachment 745051 [details] [diff] [review]
[02] Bug 831224: Allow update of the media duration

Review of attachment 745051 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/AbstractMediaDecoder.h
@@ +70,5 @@
>    // Set the duration of the media in microseconds.
>    virtual void SetMediaDuration(int64_t aDuration) = 0;
>  
> +  // Sets the duration of the media in microseconds and allows
> +  // to fire a durationchange event.

Nope, we will only fire events if this is backed by something that talks to an element.

::: content/media/MediaDecoderStateMachine.h
@@ +149,5 @@
>    // aEndTime is in microseconds.
>    void SetMediaEndTime(int64_t aEndTime);
>  
> +  // Called from decode thread to update the duration. Can result in
> +  // a durationchangeevent. aDuration is in milliseconds

|aDuration| is in microseconds here also.
Attachment #745051 - Flags: review?(paul) → review+
Comment on attachment 745052 [details] [diff] [review]
[03] Bug 831224: Parse MP3 frame headers in OmxDecoder

Review of attachment 745052 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/omx/OmxDecoder.cpp
@@ +61,5 @@
> +  NS_IMETHOD Run()
> +  {
> +    NS_ASSERTION(NS_IsMainThread(), "Should be on main thread.");
> +
> +    const char* buffer = mBuffer.get();

uint8_t

@@ +82,5 @@
> +  {
> +    MOZ_ASSERT(!NS_IsMainThread());
> +
> +    MonitorAutoLock autoLock(mCompletedMonitor);
> +    while (!mCompleted) {

Maybe an "if (" would be enough for the possible race here? This sequence happens only once.

@@ +102,5 @@
> +    mCompletedMonitor.Notify();
> +  }
> +
> +  android::sp<android::OmxDecoder> mOmxDecoder;
> +  nsAutoArrayPtr<const char>       mBuffer;

uint8_t

@@ +126,5 @@
> +  if (!length) {
> +    return NS_OK; // Cache is empty, nothing to do
> +  }
> +
> +  nsAutoArrayPtr<char> buffer(new char[length]);

uint8_t

@@ +422,5 @@
>      }
>  
> +    int64_t durationUs = -1;
> +
> +    if (!strcasecmp(audioMime, "audio/mpeg")) {

We have a bunch of constants here: http://mxr.mozilla.org/mozilla-central/source/netwerk/mime/nsMimeTypes.h#79

@@ +755,5 @@
>  
>    aSeekTimeUs = -1;
>  
>    if (err == OK && mAudioBuffer && mAudioBuffer->range_length() != 0) {
> +

Don't add an empty line here.
Attachment #745052 - Flags: review?(paul) → review+
(In reply to Thomas Zimmermann [:tzimmermann] from comment #22)
> BTW, I tested this with local MP3 files in the music app, and streamed MP3
> audio in the browser app. The duration for the local files is always
> correct. The streams start with the duration of the available data, and
> update when new data arrives.
We could add a test for that. I'm thinking simply checking the duration of a silent VBR mp3 against an hardcoded duration. See [1] and test files in the same directory for examples.

> There is some overhead for reading in the local files when the decoder
> starts, but in practice no delay was noticeable to me. If we have MP3 files
> where this becomes a problem (maybe >100 MiB) we can always split this step
> into multiple read operations.

Over 100MB mp3 files are very common when it comes to music podcasts (long duration + high bitrate). Could you try to check on a device if this is a problem?

[1]: http://mxr.mozilla.org/mozilla-central/source/content/media/test/manifest.js
Thanks for reviewing these patches. I'll address your points asap.
Created attachment 749725 [details] [diff] [review]
[01] Bug 831224: Added MP3 frame parser (v2)

I changed the MP3 parser to accept the overall length of the input stream as constructor argument. This allows to make a better guess for the duration of frames that have not been parsed.
Attachment #745050 - Attachment is obsolete: true
Attachment #749725 - Flags: review?(paul)
Created attachment 749726 [details] [diff] [review]
[02] Bug 831224: Allow update of the media duration (v2)
Attachment #745051 - Attachment is obsolete: true
Attachment #749726 - Flags: review?(paul)
Created attachment 749731 [details] [diff] [review]
[02] Bug 831224: Parse MP3 frame headers in OmxDecoder (v2)

I did the requested changes, except for the data type of the MP3 data in the OmxDecoder. All APIs (NotifyDataArrived, ReadFromCache) use char*, so I did not change the code to use uint8_t*. The overall code is cleaner this way.
Attachment #745052 - Attachment is obsolete: true
Attachment #749731 - Flags: review?(paul)
Created attachment 749737 [details] [diff] [review]
[04] Bug 831224: Use multi-threaded I/O for reading and processing MP3 frames (v2)

I did some experimentation with large MP3 files (~150 MiB) and found that it takes several seconds to read them. The solution in this patch is to read and process files in chunks of 8 MiB. The first chunk is read during initialization of the OmxDecoder and pushed to the main thread for parsing. If there is more data in the stream the main thread pushes a task for reading the next chunk to the I/O thread. This task in turn schedules the next parser runnable to the main thread. This goes on until all of the file has been read.
Attachment #749737 - Flags: review?(paul)
Comment on attachment 749725 [details] [diff] [review]
[01] Bug 831224: Added MP3 frame parser (v2)

Review of attachment 749725 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/omx/MP3FrameParser.cpp
@@ +353,5 @@
> +    if (rv == NS_OK) {
> +      mDurationUs += mp3Buffer.GetDuration();
> +      mBitRateSum += mp3Buffer.GetBitRateSum();
> +      mNumFrames  += mp3Buffer.GetNumberOfFrames();
> +      mOffset     += mp3Buffer.GetFrameSizeSum()-(mBufferLength-copyLength);

nit: spaces around mathematical operators.

::: content/media/omx/MP3FrameParser.h
@@ +37,5 @@
> +
> +class MP3FrameParser
> +{
> +public:
> +  MP3FrameParser(int64_t aLength=-1);

nit: spaces around =.
Attachment #749725 - Flags: review?(paul) → review+

Updated

5 years ago
Attachment #749726 - Flags: review?(paul) → review+

Updated

5 years ago
Attachment #749731 - Flags: review?(paul) → review+
Comment on attachment 749737 [details] [diff] [review]
[04] Bug 831224: Use multi-threaded I/O for reading and processing MP3 frames (v2)

Review of attachment 749737 [details] [diff] [review]:
-----------------------------------------------------------------

I don't have a b2g environment ready at the moment, so I can't test this properly, but I looks like it ought to work.

::: content/media/omx/OmxDecoder.cpp
@@ +61,3 @@
>  // When loading an MP3 stream from a file, we need to parse the file's
>  // content to find its duration. We must do this from within the decode
>  // thread, but parsing itself must be done in the main thread.

This is now out of date, since you can run some processing on the IO thread. Also, the next paragraph should probably be updated as well.

@@ +857,5 @@
>  
> +bool OmxDecoder::ProcessCachedData(int64_t aOffset, bool aWaitForCompletion)
> +{
> +  // We read data in chunks of 8 MiB. We can reduce this
> +  // value if media, su as sdcards, is too slow.

s/su/such/
Attachment #749737 - Flags: review?(paul) → review-
Comment on attachment 749737 [details] [diff] [review]
[04] Bug 831224: Use multi-threaded I/O for reading and processing MP3 frames (v2)

Sorry, I meant to r+ this patch.
Attachment #749737 - Flags: review- → review+
This should really have a mochitest before landing. Tell me if you need help for that.
Created attachment 752104 [details] [diff] [review]
[00] Remove trailing whitespaces
Attachment #745049 - Attachment is obsolete: true
Attachment #752104 - Flags: review+
Created attachment 752105 [details] [diff] [review]
[01] Bug 831224: Added MP3 frame parser (v2)
Attachment #749725 - Attachment is obsolete: true
Attachment #752105 - Flags: review+
Created attachment 752106 [details] [diff] [review]
[02] Bug 831224: Allow update of the media duration (v2)
Attachment #749726 - Attachment is obsolete: true
Attachment #752106 - Flags: review+
Created attachment 752107 [details] [diff] [review]
[03] Bug 831224: Parse MP3 frame headers in OmxDecoder (v2)
Attachment #749731 - Attachment is obsolete: true
Attachment #752107 - Flags: review+
Created attachment 752108 [details] [diff] [review]
[04] Bug 831224: Use multi-threaded I/O for reading and processing MP3 frames (v3)
Attachment #749737 - Attachment is obsolete: true
Attachment #752108 - Flags: review+
(In reply to Paul Adenot (:padenot) from comment #35)

Ok, thanks for the review. I fixed the typo and re-wrote the other comment to make more sense.

> This should really have a mochitest before landing. Tell me if you need help
> for that.

I just looked at some of the test html pages in the gecko source code. Can you point me to some docs, or give some infos on the topic?
Hi Paul,

Sorry for the delay here. I was busy with other stuff recently. I just tried to add a Mochitest for this bug, and I think this is already covered by content/media/test/test_played.html. The test plays files and compares the end of the played data with the overall duration. Do you think this is sufficient?
Flags: needinfo?(paul)
Yes, if you can make a file that is mostly silence, encode it in VBR so it is smaller than it should be in CBR, so the old way of computing the duration would not work, you can just add it to the list.
Flags: needinfo?(paul)
Created attachment 760880 [details] [diff] [review]
[05] Bug 831224: Added mochitest for variable-bitrate MP3s

It took me a while to assemble an MP3 file that exposes the original bug. The attached VBR-encoded mp3 file has a duration of 10 seconds. Without the patches the computed duration is only around 7 seconds.
Attachment #760880 - Flags: review?(paul)
Created attachment 760963 [details]
Failed test's screen shot

Hey Paul,

I pushed the patch set to the try server, but the MP3 file I added seems to break test_playback_rate.html.

  https://tbpl.mozilla.org/?tree=Try&rev=7035c1bca2a3

Could you help me with finding the problem? The failed test's log is available at

  https://tbpl.mozilla.org/php/getParsedLog.php?id=24007948&tree=Try

It seems like vbr.mp3 gets loaded, but not played. I also attached the generated screen shot, but it doesn't contain useful information.
Flags: needinfo?(paul)
Does the test work locally?
Flags: needinfo?(paul)
Hi Paul,

I heard of user complains regarding the bug that is fixed by these patches. So I tried to run the Mochitest again, but it still fails. However, the problem has already been reported in bug 897108; which means that it exists independently from this bug's patch set. I wanted to ask you about r-ing the Mochitest so that these changes can finally land.

Updated

5 years ago
Attachment #760880 - Flags: review?(paul) → review+
Blocks: 902476
Blocks: 902477
Blocks: 902478
Blocks: 783512
Created attachment 787573 [details] [diff] [review]
[06] Bug 831224: List vbr.mp3 in Makefile.in

The solution for the problem wit the Mochitest was rather trivial. This fixes the problem for me.
Attachment #787573 - Flags: review?(paul)
Attachment #787573 - Attachment description: Bug 831224: List vbr.mp3 in Makefile.in → [06] Bug 831224: List vbr.mp3 in Makefile.in
Comment on attachment 787573 [details] [diff] [review]
[06] Bug 831224: List vbr.mp3 in Makefile.in

Brilliant, thanks!
Attachment #787573 - Flags: review?(paul) → review+
Thanks for the quick response. If only I had known this 3 months ago... ;) Let's hope this works now.
Oh sorry missed the additional landing.

Updated

5 years ago
Depends on: 914870
See Also: → bug 1039901

Updated

4 years ago
See Also: → bug 1044772
You need to log in before you can comment on or make changes to this bug.