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

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Music, defect)

Description

[Thomas Zimmermann [:tzimmermann] [:tdz]]

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

Comment 1

[Dominic Kuo [:dkuo]]

Thomas, can you provide some link or attach your music file? thanks.

Comment 2

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: 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.

Comment 3

[Dominic Kuo [:dkuo]]

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.

Comment 4

[Thinker Li [:sinker]]

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

[cajbir (:cajbir)]

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.

Comment 6

[Thomas Zimmermann [:tzimmermann] [:tdz]]

(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.

Comment 7

[Thomas Zimmermann [:tzimmermann] [:tdz]]

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.

Comment 8

[Thomas Zimmermann [:tzimmermann] [:tdz]]

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.

Comment 9

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: 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.

Comment 10

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: 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.

Comment 11

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Hi Chris, could you give me some feedback on the attached patch or forward the request to someone else. Thanks!

Comment 12

[cajbir (:cajbir)]

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.

Comment 13

[David Scravaglieri [:scravag]]

Paul could you give a feedback to Thomas for this patch please ?

Comment 14

[Paul Adenot (:padenot)]

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.

Comment 15

[Thomas Zimmermann [:tzimmermann] [:tdz]]

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.

Comment 16

[Thomas Zimmermann [:tzimmermann] [:tdz]]

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

Comment 17

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [00] Remove trailing whitespaces

Comment 18

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [01] Bug 831224: Added MP3 frame parser

Comment 19

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [02] Bug 831224: Allow update of the media duration

Comment 20

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [03] Bug 831224: Parse MP3 frame headers in OmxDecoder

Comment 21

[Thomas Zimmermann [:tzimmermann] [:tdz]]

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.

Comment 22

[Thomas Zimmermann [:tzimmermann] [:tdz]]

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 23

[Paul Adenot (:padenot)]

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.

Comment 24

[Paul Adenot (:padenot)]

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.

Comment 25

[Paul Adenot (:padenot)]

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.

Comment 26

[Paul Adenot (:padenot)]

(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

Comment 27

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Thanks for reviewing these patches. I'll address your points asap.

Comment 28

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [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.

Comment 29

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [02] Bug 831224: Allow update of the media duration (v2)

Comment 30

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [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.

Comment 31

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [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.

Comment 32

[Paul Adenot (:padenot)]

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 =.

Comment 33

[Paul Adenot (:padenot)]

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/

Comment 34

[Paul Adenot (:padenot)]

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.

Comment 35

[Paul Adenot (:padenot)]

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

Comment 36

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [00] Remove trailing whitespaces

Comment 37

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [01] Bug 831224: Added MP3 frame parser (v2)

Comment 38

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [02] Bug 831224: Allow update of the media duration (v2)

Comment 39

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [03] Bug 831224: Parse MP3 frame headers in OmxDecoder (v2)

Comment 40

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [04] Bug 831224: Use multi-threaded I/O for reading and processing MP3 frames (v3)

Comment 41

[Thomas Zimmermann [:tzimmermann] [:tdz]]

(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?

Comment 42

[Thomas Zimmermann [:tzimmermann] [:tdz]]

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?

Comment 43

[Paul Adenot (:padenot)]

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.

Comment 44

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [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.

Comment 45

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: 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.

Comment 46

[Paul Adenot (:padenot)]

Does the test work locally?

Comment 47

[Thomas Zimmermann [:tzimmermann] [:tdz]]

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.

Comment 48

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Thanks a lot!

https://hg.mozilla.org/integration/b2g-inbound/rev/303ba353d785
https://hg.mozilla.org/integration/b2g-inbound/rev/3cfe2793f157
https://hg.mozilla.org/integration/b2g-inbound/rev/43953e9233a2
https://hg.mozilla.org/integration/b2g-inbound/rev/e334b3139e2a
https://hg.mozilla.org/integration/b2g-inbound/rev/909cddbd5af9
https://hg.mozilla.org/integration/b2g-inbound/rev/374a8aab2b49

Comment 49

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [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.

Comment 50

[Paul Adenot (:padenot)]

Comment on attachment 787573 [details] [diff] [review]
[06] Bug 831224: List vbr.mp3 in Makefile.in

Brilliant, thanks!

Comment 51

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Thanks for the quick response. If only I had known this 3 months ago... ;) Let's hope this works now.

Comment 52

[Thomas Zimmermann [:tzimmermann] [:tdz]]

For uplift:

Everything in Comment 48 and

https://hg.mozilla.org/integration/b2g-inbound/rev/952d103a1ff8

Comment 53

[Ed Morley [:emorley]]

Backed out for turning bug 897108 permaorange (see last few logs there):
remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/c2bcf2317a09
remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/4f70ae91d84e
remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/99e7a2a7029e
remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/3ee2edd9e015
remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/7e1ba78b8ab6
remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/7e8173eba326

Comment 54

[Ryan VanderMeulen [:RyanVM]]

Ironically, the backout burned the tree due to comment 52. Since M1 was actually green on the pushes prior to the backout, I just re-landed everything.

https://hg.mozilla.org/integration/b2g-inbound/rev/5b596bf93b95
https://hg.mozilla.org/integration/b2g-inbound/rev/e4a608bcb7db
https://hg.mozilla.org/integration/b2g-inbound/rev/67a2e6a133ff
https://hg.mozilla.org/integration/b2g-inbound/rev/63bfac8597fa
https://hg.mozilla.org/integration/b2g-inbound/rev/132bbc73f9ee
https://hg.mozilla.org/integration/b2g-inbound/rev/c62ea8debbdf

Comment 55

[Ed Morley [:emorley]]

Oh sorry missed the additional landing.

Comment 56

[Thomas Zimmermann [:tzimmermann] [:tdz]]

https://tbpl.mozilla.org/?tree=B2g-Inbound&rev=952d103a1ff8

included all 6 patches.

Comment 57

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/952d103a1ff8
https://hg.mozilla.org/mozilla-central/rev/5b596bf93b95
https://hg.mozilla.org/mozilla-central/rev/e4a608bcb7db
https://hg.mozilla.org/mozilla-central/rev/67a2e6a133ff
https://hg.mozilla.org/mozilla-central/rev/63bfac8597fa
https://hg.mozilla.org/mozilla-central/rev/132bbc73f9ee
https://hg.mozilla.org/mozilla-central/rev/c62ea8debbdf