1039901 - MP3FrameParser sometimes gives wrong duration on B2G

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[JW Wang [:jwwang] [:jw_wang]]

See bug bug 1023564 comment 4 for the root cause.

This bug accounts for several test failures. I spin off this bug for easier tracking the status of other test failure bugs.

Comment 1

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

copy https://bugzilla.mozilla.org/show_bug.cgi?id=1023564#c114

Timing issue:
Found the NG pattern:
First, When OmxDecoder::TryLoad is called and read the starting data, also OmxDecoderNotifyDataArrivedRunnable
was started.

Then we got: 
*MediaDecoderStateMachine::NotifyDataArrived ->notify data again, the offset = 65536, call the 
*MediaOmxReader::NotifyDataArrived ->OmxDecoder::NotifyDataArrived  
* ^^^^Decoder haven't get the first frame, so the decoder try to parse it (it maybe the 2nd frame) through *MP3FrameParser::FindNumVBRFrames function and get the wrong duration.

After that, the OmxDecoderNotifyDataArrivedRunnable notify the first frame (offset = 0) and process it by OmxDecoder::NotifyDataArrived, it would go through the MP3FrameParser::Parse and ignore by 
https://mxr.mozilla.org/mozilla-central/source/content/media/MP3FrameParser.cpp#449

Comment 2

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Attachment: patch v1

Try to reset mp3 parser if OmxDecoder can read data from cache.
https://tbpl.mozilla.org/?tree=Try&rev=5ecc3f8510e6

Comment 3

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8463192 [details] [diff] [review]
patch v1

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

::: content/media/MP3FrameParser.cpp
@@ +547,5 @@
>    //  - we look at all frames to constantly update our duration estimate.
>    return IsMP3() && !HasExactDuration();
>  }
>  
> +void MP3FrameParser::Reset()

The Reset function adds the complexity of the state management of this class. It would be simplier for the user to create another MP3FrameParser instance if he wants a fresh-new one.

::: content/media/omx/MediaOmxReader.cpp
@@ +344,4 @@
>    android::OmxDecoder *omxDecoder = mOmxDecoder.get();
>  
>    if (omxDecoder) {
> +    omxDecoder->NotifyDataArrived(aBuffer, aLength, aOffset, false);

We should totally remove MediaOmxReader::NotifyDataArrived() and let OmxDecoder figure out the MP3 duration on its own.

::: content/media/omx/OmxDecoder.cpp
@@ +619,5 @@
> +  // Avoid race condition when data comes from non-cache data at the same time. (Bug 1039901)
> +  // If we can process data from cache, use it and ignore others.
> +  if (aFromCache && aOffset == 0 && !mParseDurationByCachedData) {
> +    mParseDurationByCachedData = true;
> +    mMP3FrameParser.Reset();

It is simpler to create another instance or use placement-new to get a fresh-new object.

Comment 4

[Benjamin Chen [:bechen]]

Comment on attachment 8463192 [details] [diff] [review]
patch v1

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

I'm curious about the patch is ok or not. Because I don't see any mutex lock monitor in the patch to resolve such threading issue.
There are 2 threads, main thread from MediaDecoderStateMachine::NotifyDataArrived, another(decode thread) from OmxDecoder::TryLoad().
The 2 threads might have contention in |mOmxDecoder->NotifyDataArrived| ?

::: content/media/omx/MediaOmxReader.cpp
@@ +339,5 @@
>  }
>  
>  void MediaOmxReader::NotifyDataArrived(const char* aBuffer, uint32_t aLength, int64_t aOffset)
>  {
> +  MOZ_ASSERT(NS_IsMainThread());

Why remove this? I think the threading model doesn't change.

::: content/media/omx/OmxDecoder.h
@@ +62,5 @@
>    VideoFrame mVideoFrame;
>    AudioFrame mAudioFrame;
>    MP3FrameParser mMP3FrameParser;
>    bool mIsMp3;
> +  bool mParseDurationByCachedData;

We need an initialize value on constructor.

Comment 5

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8463192 [details] [diff] [review]
patch v1

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

::: content/media/omx/MediaOmxReader.cpp
@@ +339,5 @@
>  }
>  
>  void MediaOmxReader::NotifyDataArrived(const char* aBuffer, uint32_t aLength, int64_t aOffset)
>  {
> +  MOZ_ASSERT(NS_IsMainThread());

When MediaOmxReader::NotifyDataArrived() is called for the 1st time (aOffset == 0), it is possible |omxDecoder| is not created yet. Then |omxDecoder| will have to parse from the middle of the resource data and will get the wrong duration. It would be simpler to let OmxDecoder figure out the MP3 duration on its own by scanning the cache data.

Comment 6

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

(In reply to JW Wang [:jwwang] from comment #5)
> Comment on attachment 8463192 [details] [diff] [review]
> patch v1
> 
> Review of attachment 8463192 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/media/omx/MediaOmxReader.cpp
> @@ +339,5 @@
> >  }
> >  
> >  void MediaOmxReader::NotifyDataArrived(const char* aBuffer, uint32_t aLength, int64_t aOffset)
> >  {
> > +  MOZ_ASSERT(NS_IsMainThread());
> 
> When MediaOmxReader::NotifyDataArrived() is called for the 1st time (aOffset
> == 0), it is possible |omxDecoder| is not created yet. Then |omxDecoder|
> will have to parse from the middle of the resource data and will get the
> wrong duration. It would be simpler to let OmxDecoder figure out the MP3
> duration on its own by scanning the cache data.
As I get the NG log from log on try server, the first offset of MediaOmxReader::NotifyDataArrived isn't equal 0...
So my patch would like to process the cache data if possible.

Comment 7

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

(In reply to Benjamin Chen [:bechen] from comment #4)
> Comment on attachment 8463192 [details] [diff] [review]
> patch v1
> 
> Review of attachment 8463192 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm curious about the patch is ok or not. Because I don't see any mutex lock
> monitor in the patch to resolve such threading issue.
> There are 2 threads, main thread from
> MediaDecoderStateMachine::NotifyDataArrived, another(decode thread) from
> OmxDecoder::TryLoad().
> The 2 threads might have contention in |mOmxDecoder->NotifyDataArrived| ?
> 
> ::: content/media/omx/MediaOmxReader.cpp
> @@ +339,5 @@
> >  }
> >  
> >  void MediaOmxReader::NotifyDataArrived(const char* aBuffer, uint32_t aLength, int64_t aOffset)
> >  {
> > +  MOZ_ASSERT(NS_IsMainThread());
> 
> Why remove this? I think the threading model doesn't change.
> 
> ::: content/media/omx/OmxDecoder.h
> @@ +62,5 @@
> >    VideoFrame mVideoFrame;
> >    AudioFrame mAudioFrame;
> >    MP3FrameParser mMP3FrameParser;
> >    bool mIsMp3;
> > +  bool mParseDurationByCachedData;
> 
> We need an initialize value on constructor.

Thanks, I move the mParseDurationByCachedData to initial it in ctor.

Comment 8

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Attachment: patch v2

Sync the parsing mp3 duration logic from gstreamer.

Comment 9

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8464492 [details] [diff] [review]
patch v2

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

::: content/media/omx/MediaOmxReader.cpp
@@ +48,5 @@
>    , mHasAudio(false)
>    , mVideoSeekTimeUs(-1)
>    , mAudioSeekTimeUs(-1)
>    , mSkipCount(0)
> +  , mDataOffset(0)

This member is not used anywhere.

@@ +146,5 @@
>    EnsureActive();
>  
>    *aTags = nullptr;
>  
> +  mMP3FrameParser = new MP3FrameParser(mDecoder->GetResource()->GetLength());

You don't need the member |mMP3FrameParser|. Just make it a local and pass it to ParseMP3Headers() as needed.

@@ +174,5 @@
>    // Set the total duration (the max of the audio and video track).
>    int64_t durationUs;
> +
> +  if (isMP3 && mMP3FrameParser->IsMP3()) {
> +    // The MP3FrameParser has reported a duration; use that over the gstreamer

You need to tweak the comments here to remove the gstreamer thingy.

@@ +395,4 @@
>  void MediaOmxReader::NotifyDataArrived(const char* aBuffer, uint32_t aLength, int64_t aOffset)
>  {
> +  MOZ_ASSERT(NS_IsMainThread());
> +  if (mMP3FrameParserFromNotify.get() == nullptr) {

Just say |if (mMP3FrameParserFromNotify)|.

@@ +395,5 @@
>  void MediaOmxReader::NotifyDataArrived(const char* aBuffer, uint32_t aLength, int64_t aOffset)
>  {
> +  MOZ_ASSERT(NS_IsMainThread());
> +  if (mMP3FrameParserFromNotify.get() == nullptr) {
> +    mMP3FrameParserFromNotify = new MP3FrameParser(mDecoder->GetResource()->GetLength());

Move this if block below |if (HasVideo()| so we won't create an object this is never used when it is video.

@@ +409,5 @@
> +
> +  mMP3FrameParserFromNotify->Parse(aBuffer, aLength, aOffset);
> +
> +  int64_t duration = mMP3FrameParserFromNotify->GetDuration();
> +  if (duration > mLastParserDuration && !mUseParserDuration) {

Curious. In GStreamerReader.cpp, it says |if (duration != mLastParserDuration && mUseParserDuration)|, I am curious about why you turn mUseParserDuration into !mUseParserDuration.

Comment 10

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

(In reply to JW Wang [:jwwang] from comment #9)
> Comment on attachment 8464492 [details] [diff] [review]
> patch v2
> 
> Review of attachment 8464492 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/media/omx/MediaOmxReader.cpp
> @@ +48,5 @@
> >    , mHasAudio(false)
> >    , mVideoSeekTimeUs(-1)
> >    , mAudioSeekTimeUs(-1)
> >    , mSkipCount(0)
> > +  , mDataOffset(0)
> 
> This member is not used anywhere.
> 
> @@ +146,5 @@
> >    EnsureActive();
> >  
> >    *aTags = nullptr;
> >  
> > +  mMP3FrameParser = new MP3FrameParser(mDecoder->GetResource()->GetLength());
> 
> You don't need the member |mMP3FrameParser|. Just make it a local and pass
> it to ParseMP3Headers() as needed.
> 
> @@ +174,5 @@
> >    // Set the total duration (the max of the audio and video track).
> >    int64_t durationUs;
> > +
> > +  if (isMP3 && mMP3FrameParser->IsMP3()) {
> > +    // The MP3FrameParser has reported a duration; use that over the gstreamer
> 
> You need to tweak the comments here to remove the gstreamer thingy.
> 
> @@ +395,4 @@
> >  void MediaOmxReader::NotifyDataArrived(const char* aBuffer, uint32_t aLength, int64_t aOffset)
> >  {
> > +  MOZ_ASSERT(NS_IsMainThread());
> > +  if (mMP3FrameParserFromNotify.get() == nullptr) {
> 
> Just say |if (mMP3FrameParserFromNotify)|.
> 
> @@ +395,5 @@
> >  void MediaOmxReader::NotifyDataArrived(const char* aBuffer, uint32_t aLength, int64_t aOffset)
> >  {
> > +  MOZ_ASSERT(NS_IsMainThread());
> > +  if (mMP3FrameParserFromNotify.get() == nullptr) {
> > +    mMP3FrameParserFromNotify = new MP3FrameParser(mDecoder->GetResource()->GetLength());
> 
> Move this if block below |if (HasVideo()| so we won't create an object this
> is never used when it is video.
> 
> @@ +409,5 @@
> > +
> > +  mMP3FrameParserFromNotify->Parse(aBuffer, aLength, aOffset);
> > +
> > +  int64_t duration = mMP3FrameParserFromNotify->GetDuration();
> > +  if (duration > mLastParserDuration && !mUseParserDuration) {
> 
> Curious. In GStreamerReader.cpp, it says |if (duration !=
> mLastParserDuration && mUseParserDuration)|, I am curious about why you turn
> mUseParserDuration into !mUseParserDuration.

The logic in GStreamerReader::NotifyDataArrived also make me feel confuse....
  int64_t duration = mMP3FrameParser.GetDuration();
  if (duration != mLastParserDuration && mUseParserDuration) {
    ReentrantMonitorAutoEnter mon(mDecoder->GetReentrantMonitor());
    mLastParserDuration = duration;
    mDecoder->UpdateEstimatedMediaDuration(mLastParserDuration);
  }
1. mMP3FrameParser would also be used by GStreamerReader::ReadMetadata on decoded thread, it should have the thread safe problem. 
2. The mDecoder->UpdateEstimatedMediaDuration(mLastParserDuration); would only updated after get the duration by ReadMetadata. 
If we want to get duration through this way, it should never reach update duration...

So the newer version would remove the logic in ::NotifyDataArrived.

Comment 11

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Attachment: patch v3

Comment 12

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8465225 [details] [diff] [review]
patch v3

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

::: content/media/omx/MediaOmxReader.cpp
@@ +48,5 @@
>    , mHasAudio(false)
>    , mVideoSeekTimeUs(-1)
>    , mAudioSeekTimeUs(-1)
>    , mSkipCount(0)
> +  , mLastParserDuration(-1)

You don't need this member.

@@ +154,5 @@
>  
> +  bool isMP3 = mDecoder->GetResource()->GetContentType().EqualsASCII(AUDIO_MP3);
> +
> +  MP3FrameParser mp3FrameParser(mDecoder->GetResource()->GetLength());
> +  if (isMP3) {

Don't inline the function ParseMP3Headers() here. All you need is to pass |mp3FrameParser| as an argument to the function.

@@ +196,4 @@
>      ReentrantMonitorAutoEnter mon(mDecoder->GetReentrantMonitor());
> +    mUseParserDuration = true;
> +    mLastParserDuration = mp3FrameParser.GetDuration();
> +    mDecoder->SetMediaDuration(mLastParserDuration);

Check if the duration returned by mp3FrameParser.GetDuration() is valid before calling mDecoder->SetMediaDuration(). If not valid, fall back to call mOmxDecoder->GetDuration() below.

@@ +198,5 @@
> +    mLastParserDuration = mp3FrameParser.GetDuration();
> +    mDecoder->SetMediaDuration(mLastParserDuration);
> +  } else {
> +    mOmxDecoder->GetDuration(&durationUs);
> +    if (durationUs) {

It is prudent to check |durationUs > 0| for -1 is an invalid duration.

@@ +377,5 @@
>  
>    return true;
>  }
>  
>  void MediaOmxReader::NotifyDataArrived(const char* aBuffer, uint32_t aLength, int64_t aOffset)

Since you do nothing in this function, you don't need to override it anymore.

::: content/media/omx/MediaOmxReader.h
@@ +7,5 @@
>  #define MediaOmxReader_h_
>  
>  #include "MediaResource.h"
>  #include "MediaDecoderReader.h"
> +#include "MP3FrameParser.h"

Move this include to .cpp file.

Comment 13

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

(In reply to JW Wang [:jwwang] from comment #12)
> Comment on attachment 8465225 [details] [diff] [review]
> patch v3
> 
> Review of attachment 8465225 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/media/omx/MediaOmxReader.cpp
> @@ +48,5 @@
> >    , mHasAudio(false)
> >    , mVideoSeekTimeUs(-1)
> >    , mAudioSeekTimeUs(-1)
> >    , mSkipCount(0)
> > +  , mLastParserDuration(-1)
> 
> You don't need this member.
remove it
> 
> @@ +154,5 @@
> >  
> > +  bool isMP3 = mDecoder->GetResource()->GetContentType().EqualsASCII(AUDIO_MP3);
> > +
> > +  MP3FrameParser mp3FrameParser(mDecoder->GetResource()->GetLength());
> > +  if (isMP3) {
> 
> Don't inline the function ParseMP3Headers() here. All you need is to pass
> |mp3FrameParser| as an argument to the function.
fallback to original one (like DirectShowReader)
> @@ +196,4 @@
> >      ReentrantMonitorAutoEnter mon(mDecoder->GetReentrantMonitor());
> > +    mUseParserDuration = true;
> > +    mLastParserDuration = mp3FrameParser.GetDuration();
> > +    mDecoder->SetMediaDuration(mLastParserDuration);
> 
> Check if the duration returned by mp3FrameParser.GetDuration() is valid
> before calling mDecoder->SetMediaDuration(). If not valid, fall back to call
> mOmxDecoder->GetDuration() below.
> 
> @@ +198,5 @@
> > +    mLastParserDuration = mp3FrameParser.GetDuration();
> > +    mDecoder->SetMediaDuration(mLastParserDuration);
> > +  } else {
> > +    mOmxDecoder->GetDuration(&durationUs);
> > +    if (durationUs) {
> 
> It is prudent to check |durationUs > 0| for -1 is an invalid duration.
> 
> @@ +377,5 @@
> >  
> >    return true;
> >  }
> >  
> >  void MediaOmxReader::NotifyDataArrived(const char* aBuffer, uint32_t aLength, int64_t aOffset)
> 
> Since you do nothing in this function, you don't need to override it anymore.
> 
> ::: content/media/omx/MediaOmxReader.h
> @@ +7,5 @@
> >  #define MediaOmxReader_h_
> >  
> >  #include "MediaResource.h"
> >  #include "MediaDecoderReader.h"
> > +#include "MP3FrameParser.h"
> 
> Move this include to .cpp file.
Can't because newer version I declare the ParseMP3Headers member function in this header.
Maybe next bug would have a common location for all platform to do this.

Comment 14

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Attachment: patch 3.1

Comment 15

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8465258 [details] [diff] [review]
patch 3.1

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

Overall looks fine to me.

::: content/media/omx/MediaOmxReader.cpp
@@ +172,5 @@
>  
>    // Set the total duration (the max of the audio and video track).
>    int64_t durationUs;
> +
> +  if (isMP3 && mp3FrameParser.IsMP3()) {

Please re-arrange the code below to reduce code redundancy.
ex: mOmxDecoder->GetDuration() -> twice
ReentrantMonitorAutoEnter mon() -> twice
mDecoder->SetMediaDuration() -> twice

@@ +473,5 @@
>  }
>  #endif
>  
> +nsresult
> +MediaOmxReader::ParseMP3Headers(MP3FrameParser *aParser, MediaResource *aResource)

This should be a free function instead of a member function since it doesn't depend on any of the members. By doing so, you will be able to move |include "MP3FrameParser.h"| to the .cpp file.

Comment 16

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Attachment: patch 3.2

Hi Chris, 
Could you help to review this?
Thanks.

Comment 17

[JW Wang [:jwwang] [:jw_wang]]

I think you need to explain the idea/intention of this patch and why things are done this way. The patch involve some design changes that also need elaboration.

Comment 18

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Thanks,
1. The main idea is we want to sync the parsing logic with other platforms. 
2. The problem comes from the OmxDecoder would receive the NotifyDataAvailable callback from MSG while OmxDecoder is processing Cache data by OmxDecoderProcessCachedDataTask, and the MP3Parser isn't thread- safe, so that casue return wrong duration by race-condition.
3. Others platforms look like the parsing logic in NotifyDataAvailable also have the similar problem. We could fix that on separate bugs.

Comment 19

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8465927 [details] [diff] [review]
patch 3.2

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

Edwin wrote the MP3FrameParser, so he should review this.

::: content/media/omx/MediaOmxReader.cpp
@@ +147,5 @@
> +  uint32_t bytesRead;
> +  do {
> +    nsresult rv = aResource->ReadAt(offset, bytes, MAX_READ_BYTES, &bytesRead);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +    NS_ENSURE_TRUE(bytesRead, NS_ERROR_FAILURE);

Be careful, bytesRead==0 isn't really an error, it means you reached end of stream.

@@ +175,5 @@
> +
> +  MP3FrameParser mp3FrameParser(mDecoder->GetResource()->GetLength());
> +  if (isMP3) {
> +    MediaResource *resource = mDecoder->GetResource();
> +    ParseMP3Headers(&mp3FrameParser, resource);

ParseMP3Headers(&mp3FrameParser, mDecoder->GetResource());

You do not check the return value of ParseMP3Headers.

Perhaps here you should set:

isMP3 = NS_SUCCEEDED(ParseMP3Headers(...)) && mp3FrameParser.IsMP3();

?

@@ -337,5 @@
>  
>    return true;
>  }
>  
> -void MediaOmxReader::NotifyDataArrived(const char* aBuffer, uint32_t aLength, int64_t aOffset)

Shouldn't you be calling MP3FrameParser.Parse() in your NotifyDataArrived()? The other users of MP3FrameParser do this. Edwin can confirm this.

Comment 20

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8465927 [details] [diff] [review]
patch 3.2

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

::: content/media/omx/MediaOmxReader.cpp
@@ +147,5 @@
> +  uint32_t bytesRead;
> +  do {
> +    nsresult rv = aResource->ReadAt(offset, bytes, MAX_READ_BYTES, &bytesRead);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +    NS_ENSURE_TRUE(bytesRead, NS_ERROR_FAILURE);

Right, bytesRead==0 means end of stream generally. However, since we reach the end of stream before MP3 is successfully parsed, we return an error for ParseMP3Headers() to indicate failing to parse an MP3 file.

@@ -337,5 @@
>  
>    return true;
>  }
>  
> -void MediaOmxReader::NotifyDataArrived(const char* aBuffer, uint32_t aLength, int64_t aOffset)

That is exactly the design change we want to go for. For a cloned decoder, NotifyDataArrived() will not be called for it is not downloading any data. Therefore, we would like to remove NotifyDataArrived() for all MediaDecoderReader subclasses and instead call ParseMP3Headers() in ReadMetadata() so that the behavior is consistent across all platforms.

Comment 21

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Comment on attachment 8465927 [details] [diff] [review]
patch 3.2

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

(In reply to JW Wang [:jwwang] from comment #20)
> @@ -337,5 @@
> >  
> >    return true;
> >  }
> >  
> > -void MediaOmxReader::NotifyDataArrived(const char* aBuffer, uint32_t aLength, int64_t aOffset)
> 
> That is exactly the design change we want to go for. For a cloned decoder,
> NotifyDataArrived() will not be called for it is not downloading any data.
> Therefore, we would like to remove NotifyDataArrived() for all
> MediaDecoderReader subclasses and instead call ParseMP3Headers() in
> ReadMetadata() so that the behavior is consistent across all platforms.

Chris is correct here -- with CBR MP3s this will work fine, but with VBR MP3s we continuously update our duration estimation based on new data coming in through NotifyDataArrived.

In the case that there's no data to download, that means it should be in cache so you should be calling ReadFromCache in ReadMetadata to fill the MP3FrameParser with the cached data you have.

I would be interested to see what duration this patch gives for content/media/test/vbr.mp3

Comment 22

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

The patch on b2g flame device would report the 7s as initial duration on vbr.mp3 file. (So as Firefox browser)
Then the duration would keep increasing from 7s and ended around 9s on playing near the end of file.
Before this patch, it can report 9s at the initial of playback. (hmm, original trunk would go through the whole file and cause this: bug 1044772)

BTW, I test browser and set the break point on GStreamerReader::NotifyDataArrived, open vbr.mp3 file directly and gdb don't stop at that point while playing the whole file. I'm curious that if duration is updated by this function.

Comment 23

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Attachment: patch

Try result, 
https://tbpl.mozilla.org/?tree=Try&rev=0ba9eeee9455

Comment 24

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Comment on attachment 8467920 [details] [diff] [review]
patch

Would provide another one.

Comment 25

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Attachment: bug.patch

My idea is try to parse the mp3 stream as eaily as possible.
Try result:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=2b34842bb765

Comment 26

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Comment on attachment 8477840 [details] [diff] [review]
bug.patch

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

::: content/media/omx/MediaOmxReader.cpp
@@ +134,5 @@
> +    const char* extractorMime;
> +    sp<MetaData> meta = mExtractor->getMetaData();
> +    if (meta->findCString(kKeyMIMEType, &extractorMime) && !strcasecmp(extractorMime, AUDIO_MP3)) {
> +      int64_t length = mDecoder->GetResource()->GetCachedDataEnd(0);
> +      mMP3FrameParser.SetLength(length);

The length passed to MP3FrameParser should be the length of the whole stream, not just the cached length.

This will give an accurate length for local files, but will be completely wrong for MP3s streamed over the network.

As I noted above, you should also be calling Parse() on any data we have cached because NotifyDataArrived() is not called for cached data.

::: content/media/omx/MediaOmxReader.h
@@ +42,5 @@
>  protected:
>    android::sp<android::OmxDecoder> mOmxDecoder;
>    android::sp<android::MediaExtractor> mExtractor;
>  
> +  MP3FrameParser mMP3FrameParser;

There is already an MP3FrameParser in OmxDecoder.cpp. Why add one here as well?

Comment 27

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8477840 [details] [diff] [review]
bug.patch

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

::: content/media/omx/MediaOmxReader.cpp
@@ +45,4 @@
>    , mVideoSeekTimeUs(-1)
>    , mAudioSeekTimeUs(-1)
>    , mSkipCount(0)
> +  , mMP3FrameParser(-1)

mMP3FrameParser(aDecoder->GetResource()->GetLength()) as in GStreamerReader constructor.

@@ +134,5 @@
> +    const char* extractorMime;
> +    sp<MetaData> meta = mExtractor->getMetaData();
> +    if (meta->findCString(kKeyMIMEType, &extractorMime) && !strcasecmp(extractorMime, AUDIO_MP3)) {
> +      int64_t length = mDecoder->GetResource()->GetCachedDataEnd(0);
> +      mMP3FrameParser.SetLength(length);

As Edwin said, we need both MediaOmxReader::ReadMetadata() and something like OmxDecoder::ProcessCachedData() to handle downloaded/cached data.

@@ +350,5 @@
>  void MediaOmxReader::NotifyDataArrived(const char* aBuffer, uint32_t aLength, int64_t aOffset)
>  {
>    android::OmxDecoder *omxDecoder = mOmxDecoder.get();
> +  // Try to parse the frame
> +  if (mMP3FrameParser.IsMP3()) {

IsMP3() should be called after parsing completed to only update duration when it is confirmed to be an MP3 file.

@@ +360,2 @@
>  
>    if (omxDecoder) {

You should completely move MP3 parsing code from OmxDecoder to MediaOmxReader. if MediaOmxReader::NotifyDataArrived() is called before |mOmxDecoder| is initialized, omxDecoder->NotifyDataArrived will miss the first few bytes of the stream and MP3 parser will not give correct duration.

Comment 28

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

(In reply to JW Wang [:jwwang] from comment #27)
> Comment on attachment 8477840 [details] [diff] [review]
> bug.patch
> 
> Review of attachment 8477840 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/media/omx/MediaOmxReader.cpp
> @@ +45,4 @@
> >    , mVideoSeekTimeUs(-1)
> >    , mAudioSeekTimeUs(-1)
> >    , mSkipCount(0)
> > +  , mMP3FrameParser(-1)
> 
> mMP3FrameParser(aDecoder->GetResource()->GetLength()) as in GStreamerReader
> constructor.
> 
On B2G platform, set the length on constructor would get the length = -1. 

> @@ +134,5 @@
> > +    const char* extractorMime;
> > +    sp<MetaData> meta = mExtractor->getMetaData();
> > +    if (meta->findCString(kKeyMIMEType, &extractorMime) && !strcasecmp(extractorMime, AUDIO_MP3)) {
> > +      int64_t length = mDecoder->GetResource()->GetCachedDataEnd(0);
> > +      mMP3FrameParser.SetLength(length);
> 
> As Edwin said, we need both MediaOmxReader::ReadMetadata() and something
> like OmxDecoder::ProcessCachedData() to handle downloaded/cached data.
> 
> @@ +350,5 @@
> >  void MediaOmxReader::NotifyDataArrived(const char* aBuffer, uint32_t aLength, int64_t aOffset)
> >  {
> >    android::OmxDecoder *omxDecoder = mOmxDecoder.get();
> > +  // Try to parse the frame
> > +  if (mMP3FrameParser.IsMP3()) {
> 
> IsMP3() should be called after parsing completed to only update duration
> when it is confirmed to be an MP3 file.
The IsMP3 function returns false if the mp3Parser detect it's NOT valid mp3 frame,
At initial time, the inner state would be MAYBE_MP3.
> 
> @@ +360,2 @@
> >  
> >    if (omxDecoder) {
> 
> You should completely move MP3 parsing code from OmxDecoder to
> MediaOmxReader. if MediaOmxReader::NotifyDataArrived() is called before
> |mOmxDecoder| is initialized, omxDecoder->NotifyDataArrived will miss the
> first few bytes of the stream and MP3 parser will not give correct duration.
For this reason, I add another MP3 parser instance in reader object.
So if the decoder haven't constructed.
I still can parse the first several frames and pass some addition information to decoder.
The running sequence in try server is.
1. MSG notify data to reader
frame 1 frame 2 frame 3...........frame N (offset X)......
------decoder not ready-----decoder ready---------(1)
                                                 -----frame 1  frame 2 ...frame X 
                                                    ^^^(2)
(1) data from MSG, may start parse from offset=12345.
(2) data form OmxDecoderNotifyDataArrivedRunnable, want to parse data from offset = 0
on (2) case, because MP3FrameParser can't accept back-forward offset frame data,  those data from (2)  on slower machine would be ignore in MP3FrameParser, then we get bias duration relay on few frame samples static data. 

For this question: 
-->As I noted above, you should also be calling Parse() on any data we have cached because NotifyDataArrived() is not called for cached data.
I didn't remove the ProcessCachedData logic, it should be processed. 


If we want to get the more accuracy mp3 duration, 
I think 
1. Get the actual stream length.
(cached length isn't work, Should I use the aDecoder->GetResource()->GetLength() ?)

2. Parsing frames as many as possible, and count the static data. (Maybe refine the MP3 parser to support feed any position of frame data, not only parse frames in sequence.)

Comment 29

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

(In reply to Randy Lin [:rlin] from comment #28)
> > @@ +360,2 @@
> > >  
> > >    if (omxDecoder) {
> > 
> > You should completely move MP3 parsing code from OmxDecoder to
> > MediaOmxReader. if MediaOmxReader::NotifyDataArrived() is called before
> > |mOmxDecoder| is initialized, omxDecoder->NotifyDataArrived will miss the
> > first few bytes of the stream and MP3 parser will not give correct duration.
> For this reason, I add another MP3 parser instance in reader object.
> So if the decoder haven't constructed.
> I still can parse the first several frames and pass some addition
> information to decoder.

Yes, adding an MP3FrameParser to MediaOmxReader is definitely a good thing. What I (and I assume JWWang) mean is that this makes the MP3FrameParser in OmxDecoder redundant; so we can remove the parser from OmxDecoder.

> For this question: 
> -->As I noted above, you should also be calling Parse() on any data we have
> cached because NotifyDataArrived() is not called for cached data.
> I didn't remove the ProcessCachedData logic, it should be processed. 

ProcessCachedData exists in OmxDecoder, but there's nothing like it in MediaOmxDecoder.

> 1. Get the actual stream length.
> (cached length isn't work, Should I use the
> aDecoder->GetResource()->GetLength() ?)

Yes, that should give you the correct stream length if it is known.

> 2. Parsing frames as many as possible, and count the static data. (Maybe
> refine the MP3 parser to support feed any position of frame data, not only
> parse frames in sequence.)

The MP3FrameParser already supports this.

Comment 30

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

(In reply to Edwin Flores [:eflores] [:edwin] from comment #29)
> (In reply to Randy Lin [:rlin] from comment #28)
> > > @@ +360,2 @@
> > > >  
> > > >    if (omxDecoder) {
> > > 
> > > You should completely move MP3 parsing code from OmxDecoder to
> > > MediaOmxReader. if MediaOmxReader::NotifyDataArrived() is called before
> > > |mOmxDecoder| is initialized, omxDecoder->NotifyDataArrived will miss the
> > > first few bytes of the stream and MP3 parser will not give correct duration.
> > For this reason, I add another MP3 parser instance in reader object.
> > So if the decoder haven't constructed.
> > I still can parse the first several frames and pass some addition
> > information to decoder.
> 
> Yes, adding an MP3FrameParser to MediaOmxReader is definitely a good thing.
> What I (and I assume JWWang) mean is that this makes the MP3FrameParser in
> OmxDecoder redundant; so we can remove the parser from OmxDecoder.
> 
> > For this question: 
> > -->As I noted above, you should also be calling Parse() on any data we have
> > cached because NotifyDataArrived() is not called for cached data.
> > I didn't remove the ProcessCachedData logic, it should be processed. 
> 
> ProcessCachedData exists in OmxDecoder, but there's nothing like it in
> MediaOmxDecoder.
> 
> > 1. Get the actual stream length.
> > (cached length isn't work, Should I use the
> > aDecoder->GetResource()->GetLength() ?)
> 
> Yes, that should give you the correct stream length if it is known.
> 
> > 2. Parsing frames as many as possible, and count the static data. (Maybe
> > refine the MP3 parser to support feed any position of frame data, not only
> > parse frames in sequence.)
> 
> The MP3FrameParser already supports this.
Refer to 
http://lxr.mozilla.org/mozilla-central/source/content/media/MP3FrameParser.cpp#448
If MP3FrameParser::Parse() try to parse the frame's offset less and not overlap the previous data offset, 
This function would just return.
That why I said the MP3FrameParser doesn't support feed any position of frame data.

Comment 31

[JW Wang [:jwwang] [:jw_wang]]

See http://lxr.mozilla.org/mozilla-central/source/content/media/MP3FrameParser.cpp#455.

When there is a discontinuity in the input stream, MP3FrameParser will reset and discard the duration accumulated so far. This is not very ideal to me.

We should either 1. ensure there is no gap in the bytes fed to MP3FrameParser to avoid the situation above or 2. make MP3FrameParser work a little more harder to handle the discontinuity in the input stream to give as more precise duration as possible.

Comment 32

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Attachment: b2g.patch v1

This patch is moving the mp3 parsing logic from OmxDecoder to MediaOmxReader. 
That can avoid the init delay of omxDecoder in NotifyDataArrived function.

Comment 33

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8486182 [details] [diff] [review]
b2g.patch v1

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

Overall looks fine. Please address the issues in my comments.

::: content/media/MP3FrameParser.h
@@ +132,5 @@
>  
>    // Returns true if the parser needs more data for duration estimation.
>    bool NeedsData();
> +  // Assign the total lenght of this mp3 stream
> +  void SetLength(int64_t aLength) { mLength = aLength; }

Must hold the lock before changing the member.

::: content/media/omx/MediaOmxReader.cpp
@@ +283,5 @@
>    }
>  
> +  if (isMP3 && mMP3FrameParser.IsMP3()) {
> +    // The MP3FrameParser has reported a duration; use that over the gstreamer
> +    // reported duration for inter-platform consistency.

1. fix the comment which contains "gstreamer"
2. the comment seems wrong for mMP3FrameParser.IsMP3() could return true even without parsing any data.

@@ +288,4 @@
>      ReentrantMonitorAutoEnter mon(mDecoder->GetReentrantMonitor());
> +    mUseParserDuration = true;
> +    mLastParserDuration = mMP3FrameParser.GetDuration();
> +    mDecoder->SetMediaDuration(mLastParserDuration);

We should only call mDecoder->SetMediaDuration() when mLastParserDuration is a valid duration for mMP3FrameParser.GetDuration() could be negative.

@@ +480,5 @@
> +  }
> +
> +  mMP3FrameParser.Parse(aBuffer, aLength, aOffset);
> +  int64_t duration = mMP3FrameParser.GetDuration();
> +  if (duration != mLastParserDuration && mUseParserDuration) {

1. Need to hold the monitor before accessing |mLastParserDuration| or |mUseParserDuration|.
2. It is safer to check duration > mLastParserDuration for mMP3FrameParser.GetDuration() could return a negative value.

@@ +598,5 @@
> +                                           bufferLength,
> +                                           aOffset,
> +                                           resourceLength));
> +
> +  rv = NS_DispatchToMainThread(runnable.get());

Pass NS_DISPATCH_SYNC flag for sync dispatch to save the implementation on your own.

Comment 34

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Attachment: mp3parser.patch v1

This patch try to let mp3 parser to accept non-sequence offset frame data.

Comment 35

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Attachment: test_playback.html.patch

Enable the mp3 duration check in test_playback.html.

Comment 36

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8486251 [details] [diff] [review]
test_playback.html.patch

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

localCheckMetadata() is basically as copy of checkMetadata() in manifest.js with MP3 workarounds. You just need to remove the function localCheckMetadata() and the line http://hg.mozilla.org/mozilla-central/file/4d1793da0b96/content/media/test/test_playback.html#l43. Then the checkMetadata() in manifest.js will be used again.

Comment 37

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Attachment: bug.patch v2

Comment 38

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Comment on attachment 8486925 [details] [diff] [review]
bug.patch v2

Thanks JW, 
Fix nits.

Comment 39

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Attachment: bug.patch

Sorry, mix the test_playback.html patch. update new one.

Comment 40

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Comment on attachment 8486926 [details] [diff] [review]
bug.patch

># HG changeset patch
># User Randy Lin <[e-mail]>
># Date 1410233332 -28800
># Node ID 32a7d8ca4b1d8079aa4b3f2279be79b2353962ee
># Parent  152ef25e89aeacb9cf1046dd926520fdbe407b85
>Bug 1039901 - Part 1. MP3FrameParser sometimes gives wrong duration on B2G.  Move the ProcessCacheData function from OmxDecoder to MediaOmxReader.
>
>diff --git a/content/media/MP3FrameParser.h b/content/media/MP3FrameParser.h
>--- a/content/media/MP3FrameParser.h
>+++ b/content/media/MP3FrameParser.h
>@@ -127,17 +127,21 @@ public:
>   bool ParsedHeaders();
> 
>   // Returns true if we know the exact duration of the MP3 stream;
>   // false otherwise.
>   bool HasExactDuration();
> 
>   // Returns true if the parser needs more data for duration estimation.
>   bool NeedsData();
>-
>+  // Assign the total lenght of this mp3 stream
>+  void SetLength(int64_t aLength) {
>+    MutexAutoLock mon(mLock);
>+    mLength = aLength;
>+  }
> private:
> 
>   // Parses aBuffer, starting at offset 0. Returns the number of bytes
>   // parsed, relative to the start of the buffer. Note this may be
>   // greater than aLength if the headers in the buffer indicate that
>   // the frame or ID3 tag extends outside of aBuffer. Returns failure
>   // if too many non-MP3 bytes are parsed.
>   nsresult ParseBuffer(const uint8_t* aBuffer,
>diff --git a/content/media/omx/MediaOmxReader.cpp b/content/media/omx/MediaOmxReader.cpp
>--- a/content/media/omx/MediaOmxReader.cpp
>+++ b/content/media/omx/MediaOmxReader.cpp
>@@ -30,23 +30,142 @@ namespace mozilla {
> 
> #ifdef PR_LOGGING
> extern PRLogModuleInfo* gMediaDecoderLog;
> #define DECODER_LOG(type, msg) PR_LOG(gMediaDecoderLog, type, msg)
> #else
> #define DECODER_LOG(type, msg)
> #endif
> 
>+class OmxReaderProcessCachedDataTask : public Task
>+{
>+public:
>+  OmxReaderProcessCachedDataTask(MediaOmxReader* aOmxReader, int64_t aOffset)
>+  : mOmxReader(aOmxReader),
>+    mOffset(aOffset)
>+  { }
>+
>+  void Run()
>+  {
>+    MOZ_ASSERT(!NS_IsMainThread());
>+    MOZ_ASSERT(mOmxReader.get());
>+    mOmxReader->ProcessCachedData(mOffset, false);
>+  }
>+
>+private:
>+  nsRefPtr<MediaOmxReader> mOmxReader;
>+  int64_t                  mOffset;
>+};
>+
>+// When loading an MP3 stream from a file, we need to parse the file's
>+// content to find its duration. Reading files of 100 MiB or more can
>+// delay the player app noticably, so the file is read and decoded in
>+// smaller chunks.
>+//
>+// We first read on the decode thread, but parsing must be done on the
>+// main thread. After we read the file's initial MiBs in the decode
>+// thread, an instance of this class is scheduled to the main thread for
>+// parsing the MP3 stream. The decode thread waits until it has finished.
>+//
>+// If there is more data available from the file, the runnable dispatches
>+// a task to the IO thread for retrieving the next chunk of data, and
>+// the IO task dispatches a runnable to the main thread for parsing the
>+// data. This goes on until all of the MP3 file has been parsed.
>+
>+class OmxReaderNotifyDataArrivedRunnable : public nsRunnable
>+{
>+public:
>+  OmxReaderNotifyDataArrivedRunnable(MediaOmxReader* aOmxReader,
>+                                     const char* aBuffer, uint64_t aLength,
>+                                     int64_t aOffset, uint64_t aFullLength)
>+  : mOmxReader(aOmxReader),
>+    mBuffer(aBuffer),
>+    mLength(aLength),
>+    mOffset(aOffset),
>+    mFullLength(aFullLength),
>+    mCompletedMonitor("OmxReaderNotifyDataArrived.mCompleted"),
>+    mCompleted(false)
>+  {
>+    MOZ_ASSERT(mOmxReader.get());
>+    MOZ_ASSERT(mBuffer.get() || !mLength);
>+  }
>+
>+  NS_IMETHOD Run()
>+  {
>+    NS_ASSERTION(NS_IsMainThread(), "Should be on main thread.");
>+
>+    NotifyDataArrived();
>+    Completed();
>+
>+    return NS_OK;
>+  }
>+
>+  void WaitForCompletion()
>+  {
>+    MOZ_ASSERT(!NS_IsMainThread());
>+
>+    MonitorAutoLock mon(mCompletedMonitor);
>+    if (!mCompleted) {
>+      mCompletedMonitor.Wait();
>+    }
>+  }
>+
>+private:
>+  void NotifyDataArrived()
>+  {
>+    const char* buffer = mBuffer.get();
>+
>+    while (mLength) {
>+      uint32_t length = std::min<uint64_t>(mLength, UINT32_MAX);
>+      mOmxReader->NotifyDataArrived(buffer, length,
>+                                    mOffset);
>+      buffer  += length;
>+      mLength -= length;
>+      mOffset += length;
>+    }
>+
>+    if (mOffset < mFullLength) {
>+      // We cannot read data in the main thread because it
>+      // might block for too long. Instead we post an IO task
>+      // to the IO thread if there is more data available.
>+      XRE_GetIOMessageLoop()->PostTask(FROM_HERE,
>+          new OmxReaderProcessCachedDataTask(mOmxReader.get(), mOffset));
>+    }
>+  }
>+
>+  // Call this function at the end of Run() to notify waiting
>+  // threads.
>+  void Completed()
>+  {
>+    MonitorAutoLock mon(mCompletedMonitor);
>+    MOZ_ASSERT(!mCompleted);
>+    mCompleted = true;
>+    mCompletedMonitor.Notify();
>+  }
>+
>+  nsRefPtr<MediaOmxReader> mOmxReader;
>+  nsAutoArrayPtr<const char>       mBuffer;
>+  uint64_t                         mLength;
>+  int64_t                          mOffset;
>+  uint64_t                         mFullLength;
>+
>+  Monitor mCompletedMonitor;
>+  bool    mCompleted;
>+};
>+
> MediaOmxReader::MediaOmxReader(AbstractMediaDecoder *aDecoder)
>   : MediaOmxCommonReader(aDecoder)
>+  , mMP3FrameParser(-1)
>   , mHasVideo(false)
>   , mHasAudio(false)
>   , mVideoSeekTimeUs(-1)
>   , mAudioSeekTimeUs(-1)
>   , mSkipCount(0)
>+  , mUseParserDuration(false)
>+  , mLastParserDuration(-1)
> {
> #ifdef PR_LOGGING
>   if (!gMediaDecoderLog) {
>     gMediaDecoderLog = PR_NewLogModule("MediaDecoder");
>   }
> #endif
> 
>   mAudioChannel = dom::AudioChannelService::GetDefaultAudioChannel();
>@@ -129,45 +248,64 @@ nsresult MediaOmxReader::InitOmxDecoder(
>   return NS_OK;
> }
> 
> nsresult MediaOmxReader::ReadMetadata(MediaInfo* aInfo,
>                                       MetadataTags** aTags)
> {
>   NS_ASSERTION(mDecoder->OnDecodeThread(), "Should be on decode thread.");
>   EnsureActive();
>-
>   *aTags = nullptr;
> 
>   // Initialize the internal OMX Decoder.
>   nsresult rv = InitOmxDecoder();
>   if (NS_FAILED(rv)) {
>     return rv;
>   }
> 
>+  bool isMP3 = mDecoder->GetResource()->GetContentType().EqualsASCII(AUDIO_MP3);
>+  if (isMP3) {
>+    // When read sdcard's file on b2g platform at constructor,
>+    // the mDecoder->GetResource()->GetLength() would return -1.
>+    // Delay set the total duration on this function.
>+    mMP3FrameParser.SetLength(mDecoder->GetResource()->GetLength());
>+    ProcessCachedData(0, true);
>+  }
>+
>   if (!mOmxDecoder->TryLoad()) {
>     return NS_ERROR_FAILURE;
>   }
> 
> #ifdef MOZ_AUDIO_OFFLOAD
>   CheckAudioOffload();
> #endif
> 
>   if (IsWaitingMediaResources()) {
>     return NS_OK;
>   }
> 
>-  // Set the total duration (the max of the audio and video track).
>-  int64_t durationUs;
>-  mOmxDecoder->GetDuration(&durationUs);
>-  if (durationUs) {
>-    ReentrantMonitorAutoEnter mon(mDecoder->GetReentrantMonitor());
>-    mDecoder->SetMediaDuration(durationUs);
>+  if (isMP3 && mMP3FrameParser.IsMP3()) {
>+    int64_t duration = mMP3FrameParser.GetDuration();
>+    // The MP3FrameParser may reported a duration;
>+    // return -1 if no frame has been parsed.
>+    if (duration >= 0) {
>+      ReentrantMonitorAutoEnter mon(mDecoder->GetReentrantMonitor());
>+      mUseParserDuration = true;
>+      mLastParserDuration = mMP3FrameParser.GetDuration();
>+      mDecoder->SetMediaDuration(mLastParserDuration);
>+    }
>+  } else {
>+    // Set the total duration (the max of the audio and video track).
>+    int64_t durationUs;
>+    mOmxDecoder->GetDuration(&durationUs);
>+    if (durationUs) {
>+      ReentrantMonitorAutoEnter mon(mDecoder->GetReentrantMonitor());
>+      mDecoder->SetMediaDuration(durationUs);
>+    }
>   }
>-
>   if (mOmxDecoder->HasVideo()) {
>     int32_t displayWidth, displayHeight, width, height;
>     mOmxDecoder->GetVideoParameters(&displayWidth, &displayHeight,
>                                     &width, &height);
>     nsIntRect pictureRect(0, 0, width, height);
> 
>     // Validate the container-reported frame and pictureRect sizes. This ensures
>     // that our video frame creation code doesn't overflow.
>@@ -329,20 +467,32 @@ bool MediaOmxReader::DecodeVideoFrame(bo
>     break;
>   }
> 
>   return true;
> }
> 
> void MediaOmxReader::NotifyDataArrived(const char* aBuffer, uint32_t aLength, int64_t aOffset)
> {
>-  android::OmxDecoder *omxDecoder = mOmxDecoder.get();
>+  MOZ_ASSERT(NS_IsMainThread());
> 
>-  if (omxDecoder) {
>-    omxDecoder->NotifyDataArrived(aBuffer, aLength, aOffset);
>+  if (HasVideo()) {
>+    return;
>+  }
>+
>+  if (!mMP3FrameParser.NeedsData()) {
>+    return;
>+  }
>+
>+  mMP3FrameParser.Parse(aBuffer, aLength, aOffset);
>+  int64_t duration = mMP3FrameParser.GetDuration();
>+  ReentrantMonitorAutoEnter mon(mDecoder->GetReentrantMonitor());
>+  if (duration != mLastParserDuration && mUseParserDuration) {
>+    mLastParserDuration = duration;
>+    mDecoder->UpdateEstimatedMediaDuration(mLastParserDuration);
>   }
> }
> 
> bool MediaOmxReader::DecodeAudioData()
> {
>   NS_ASSERTION(mDecoder->OnDecodeThread(), "Should be on decode thread.");
>   EnsureActive();
> 
>@@ -414,16 +564,59 @@ void MediaOmxReader::SetIdle() {
> void MediaOmxReader::EnsureActive() {
>   if (!mOmxDecoder.get()) {
>     return;
>   }
>   DebugOnly<nsresult> result = mOmxDecoder->Play();
>   NS_ASSERTION(result == NS_OK, "OmxDecoder should be in play state to continue decoding");
> }
> 
>+int64_t MediaOmxReader::ProcessCachedData(int64_t aOffset, bool aWaitForCompletion)
>+{
>+  // We read data in chunks of 32 KiB. We can reduce this
>+  // value if media, such as sdcards, is too slow.
>+  // Because of SD card's slowness, need to keep sReadSize to small size.
>+  // See Bug 914870.
>+  static const int64_t sReadSize = 32 * 1024;
>+
>+  NS_ASSERTION(!NS_IsMainThread(), "Should not be on main thread.");
>+
>+  MOZ_ASSERT(mDecoder->GetResource());
>+  int64_t resourceLength = mDecoder->GetResource()->GetCachedDataEnd(0);
>+  NS_ENSURE_TRUE(resourceLength >= 0, -1);
>+
>+  if (aOffset >= resourceLength) {
>+    return 0; // Cache is empty, nothing to do
>+  }
>+
>+  int64_t bufferLength = std::min<int64_t>(resourceLength-aOffset, sReadSize);
>+
>+  nsAutoArrayPtr<char> buffer(new char[bufferLength]);
>+
>+  nsresult rv = mDecoder->GetResource()->ReadFromCache(buffer.get(),
>+                                                       aOffset, bufferLength);
>+  NS_ENSURE_SUCCESS(rv, -1);
>+
>+  nsRefPtr<OmxReaderNotifyDataArrivedRunnable> runnable(
>+    new OmxReaderNotifyDataArrivedRunnable(this,
>+                                           buffer.forget(),
>+                                           bufferLength,
>+                                           aOffset,
>+                                           resourceLength));
>+
>+  rv = NS_DispatchToMainThread(runnable.get(), NS_DISPATCH_SYNC);
>+  NS_ENSURE_SUCCESS(rv, -1);
>+
>+  if (aWaitForCompletion) {
>+    runnable->WaitForCompletion();
>+  }
>+
>+  return resourceLength - aOffset - bufferLength;
>+}
>+
> android::sp<android::MediaSource> MediaOmxReader::GetAudioOffloadTrack()
> {
>   if (!mOmxDecoder.get()) {
>     return nullptr;
>   }
>   return mOmxDecoder->GetAudioOffloadTrack();
> }
> 
>diff --git a/content/media/omx/MediaOmxReader.h b/content/media/omx/MediaOmxReader.h
>--- a/content/media/omx/MediaOmxReader.h
>+++ b/content/media/omx/MediaOmxReader.h
>@@ -4,16 +4,18 @@
>  * 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/. */
> #if !defined(MediaOmxReader_h_)
> #define MediaOmxReader_h_
> 
> #include "MediaOmxCommonReader.h"
> #include "MediaResource.h"
> #include "MediaDecoderReader.h"
>+#include "nsMimeTypes.h"
>+#include "MP3FrameParser.h"
> #include "nsRect.h"
> #include <ui/GraphicBuffer.h>
> #include <stagefright/MediaSource.h>
> 
> namespace android {
> class OmxDecoder;
> class MOZ_EXPORT MediaExtractor;
> }
>@@ -30,22 +32,23 @@ class MediaOmxReader : public MediaOmxCo
> {
>   nsCString mType;
>   bool mHasVideo;
>   bool mHasAudio;
>   nsIntRect mPicture;
>   nsIntSize mInitialFrame;
>   int64_t mVideoSeekTimeUs;
>   int64_t mAudioSeekTimeUs;
>+  int64_t mLastParserDuration;
>   int32_t mSkipCount;
>-
>+  bool mUseParserDuration;
> protected:
>   android::sp<android::OmxDecoder> mOmxDecoder;
>   android::sp<android::MediaExtractor> mExtractor;
>-
>+  MP3FrameParser mMP3FrameParser;
>   // Called by ReadMetadata() during MediaDecoderStateMachine::DecodeMetadata()
>   // on decode thread. It create and initialize the OMX decoder including
>   // setting up custom extractor. The extractor provide the essential
>   // information used for creating OMX decoder such as video/audio codec.
>   virtual nsresult InitOmxDecoder();
> 
>   // Called inside DecodeVideoFrame, DecodeAudioData, ReadMetadata and Seek
>   // to activate the decoder automatically.
>@@ -85,14 +88,16 @@ public:
>   virtual bool IsMediaSeekable() MOZ_OVERRIDE;
> 
>   virtual void SetIdle() MOZ_OVERRIDE;
> 
>   virtual void Shutdown() MOZ_OVERRIDE;
> 
>   void ReleaseDecoder();
> 
>+  int64_t ProcessCachedData(int64_t aOffset, bool aWaitForCompletion);
>+
>   android::sp<android::MediaSource> GetAudioOffloadTrack();
> };
> 
> } // namespace mozilla
> 
> #endif
>diff --git a/content/media/omx/OmxDecoder.cpp b/content/media/omx/OmxDecoder.cpp
>--- a/content/media/omx/OmxDecoder.cpp
>+++ b/content/media/omx/OmxDecoder.cpp
>@@ -39,166 +39,16 @@ PRLogModuleInfo *gOmxDecoderLog;
> #else
> #define LOG(x...)
> #endif
> 
> using namespace MPAPI;
> using namespace mozilla;
> using namespace mozilla::gfx;
> using namespace mozilla::layers;
>-
>-namespace mozilla {
>-
>-class ReleaseOmxDecoderRunnable : public nsRunnable
>-{
>-public:
>-  ReleaseOmxDecoderRunnable(const android::sp<android::OmxDecoder>& aOmxDecoder)
>-  : mOmxDecoder(aOmxDecoder)
>-  {
>-  }
>-
>-  NS_METHOD Run() MOZ_OVERRIDE
>-  {
>-    MOZ_ASSERT(NS_IsMainThread());
>-    mOmxDecoder = nullptr; // release OmxDecoder
>-    return NS_OK;
>-  }
>-
>-private:
>-  android::sp<android::OmxDecoder> mOmxDecoder;
>-};
>-
>-class OmxDecoderProcessCachedDataTask : public Task
>-{
>-public:
>-  OmxDecoderProcessCachedDataTask(android::OmxDecoder* aOmxDecoder, int64_t aOffset)
>-  : mOmxDecoder(aOmxDecoder),
>-    mOffset(aOffset)
>-  { }
>-
>-  void Run()
>-  {
>-    MOZ_ASSERT(!NS_IsMainThread());
>-    MOZ_ASSERT(mOmxDecoder.get());
>-    int64_t rem = mOmxDecoder->ProcessCachedData(mOffset, false);
>-
>-    if (rem <= 0) {
>-      ReleaseOmxDecoderRunnable* r = new ReleaseOmxDecoderRunnable(mOmxDecoder);
>-      mOmxDecoder.clear();
>-      NS_DispatchToMainThread(r);
>-    }
>-  }
>-
>-private:
>-  android::sp<android::OmxDecoder> mOmxDecoder;
>-  int64_t                          mOffset;
>-};
>-
>-// When loading an MP3 stream from a file, we need to parse the file's
>-// content to find its duration. Reading files of 100 MiB or more can
>-// delay the player app noticably, so the file is read and decoded in
>-// smaller chunks.
>-//
>-// We first read on the decode thread, but parsing must be done on the
>-// main thread. After we read the file's initial MiBs in the decode
>-// thread, an instance of this class is scheduled to the main thread for
>-// parsing the MP3 stream. The decode thread waits until it has finished.
>-//
>-// If there is more data available from the file, the runnable dispatches
>-// a task to the IO thread for retrieving the next chunk of data, and
>-// the IO task dispatches a runnable to the main thread for parsing the
>-// data. This goes on until all of the MP3 file has been parsed.
>-
>-class OmxDecoderNotifyDataArrivedRunnable : public nsRunnable
>-{
>-public:
>-  OmxDecoderNotifyDataArrivedRunnable(android::OmxDecoder* aOmxDecoder,
>-                                      const char* aBuffer, uint64_t aLength,
>-                                      int64_t aOffset, uint64_t aFullLength)
>-  : mOmxDecoder(aOmxDecoder),
>-    mBuffer(aBuffer),
>-    mLength(aLength),
>-    mOffset(aOffset),
>-    mFullLength(aFullLength),
>-    mCompletedMonitor("OmxDecoderNotifyDataArrived.mCompleted"),
>-    mCompleted(false)
>-  {
>-    MOZ_ASSERT(mOmxDecoder.get());
>-    MOZ_ASSERT(mBuffer.get() || !mLength);
>-  }
>-
>-  NS_IMETHOD Run()
>-  {
>-    NS_ASSERTION(NS_IsMainThread(), "Should be on main thread.");
>-
>-    NotifyDataArrived();
>-    Completed();
>-
>-    return NS_OK;
>-  }
>-
>-  void WaitForCompletion()
>-  {
>-    MOZ_ASSERT(!NS_IsMainThread());
>-
>-    MonitorAutoLock mon(mCompletedMonitor);
>-    if (!mCompleted) {
>-      mCompletedMonitor.Wait();
>-    }
>-  }
>-
>-private:
>-  void NotifyDataArrived()
>-  {
>-    const char* buffer = mBuffer.get();
>-
>-    while (mLength) {
>-      uint32_t length = std::min<uint64_t>(mLength, UINT32_MAX);
>-      bool success = mOmxDecoder->NotifyDataArrived(buffer, mLength,
>-                                                    mOffset);
>-      if (!success) {
>-        return;
>-      }
>-
>-      buffer  += length;
>-      mLength -= length;
>-      mOffset += length;
>-    }
>-
>-    if (mOffset < mFullLength) {
>-      // We cannot read data in the main thread because it
>-      // might block for too long. Instead we post an IO task
>-      // to the IO thread if there is more data available.
>-      XRE_GetIOMessageLoop()->PostTask(FROM_HERE,
>-          new OmxDecoderProcessCachedDataTask(mOmxDecoder.get(), mOffset));
>-    }
>-  }
>-
>-  // Call this function at the end of Run() to notify waiting
>-  // threads.
>-  void Completed()
>-  {
>-    MonitorAutoLock mon(mCompletedMonitor);
>-    MOZ_ASSERT(!mCompleted);
>-    mCompleted = true;
>-    mCompletedMonitor.Notify();
>-  }
>-
>-  android::sp<android::OmxDecoder> mOmxDecoder;
>-  nsAutoArrayPtr<const char>       mBuffer;
>-  uint64_t                         mLength;
>-  int64_t                          mOffset;
>-  uint64_t                         mFullLength;
>-
>-  Monitor mCompletedMonitor;
>-  bool    mCompleted;
>-};
>-
>-}
>-
> using namespace android;
> 
> OmxDecoder::OmxDecoder(MediaResource *aResource,
>                        AbstractMediaDecoder *aDecoder) :
>   mDecoder(aDecoder),
>   mResource(aResource),
>   mDisplayWidth(0),
>   mDisplayHeight(0),
>@@ -206,18 +56,16 @@ OmxDecoder::OmxDecoder(MediaResource *aR
>   mVideoHeight(0),
>   mVideoColorFormat(0),
>   mVideoStride(0),
>   mVideoSliceHeight(0),
>   mVideoRotation(0),
>   mAudioChannels(-1),
>   mAudioSampleRate(-1),
>   mDurationUs(-1),
>-  mMP3FrameParser(aResource->GetLength()),
>-  mIsMp3(false),
>   mVideoBuffer(nullptr),
>   mAudioBuffer(nullptr),
>   mIsVideoSeeking(false),
>   mAudioMetadataRead(false),
>   mAudioPaused(false),
>   mVideoPaused(false)
> {
>   mLooper = new ALooper;
>@@ -263,19 +111,16 @@ bool OmxDecoder::Init(sp<MediaExtractor>
> #ifdef PR_LOGGING
>   if (!gOmxDecoderLog) {
>     gOmxDecoderLog = PR_NewLogModule("OmxDecoder");
>   }
> #endif
> 
>   const char* extractorMime;
>   sp<MetaData> meta = extractor->getMetaData();
>-  if (meta->findCString(kKeyMIMEType, &extractorMime) && !strcasecmp(extractorMime, AUDIO_MP3)) {
>-    mIsMp3 = true;
>-  }
> 
>   ssize_t audioTrackIndex = -1;
>   ssize_t videoTrackIndex = -1;
> 
>   for (size_t i = 0; i < extractor->countTracks(); ++i) {
>     sp<MetaData> meta = extractor->getTrackMetaData(i);
> 
>     int32_t bitRate;
>@@ -337,27 +182,16 @@ bool OmxDecoder::TryLoad() {
>     if (durationUs > totalDurationUs)
>       totalDurationUs = durationUs;
>   }
>   if (mAudioTrack.get()) {
>     durationUs = -1;
>     const char* audioMime;
>     sp<MetaData> meta = mAudioTrack->getFormat();
> 
>-    if (mIsMp3) {
>-      // Feed MP3 parser with cached data. Local files will be fully
>-      // cached already, network streams will update with sucessive
>-      // calls to NotifyDataArrived.
>-      if (ProcessCachedData(0, true) >= 0) {
>-        durationUs = mMP3FrameParser.GetDuration();
>-        if (durationUs > totalDurationUs) {
>-          totalDurationUs = durationUs;
>-        }
>-      }
>-    }
>     if ((durationUs == -1) && meta->findInt64(kKeyDuration, &durationUs)) {
>       if (durationUs > totalDurationUs) {
>         totalDurationUs = durationUs;
>       }
>     }
>   }
>   mDurationUs = totalDurationUs;
> 
>@@ -630,37 +464,16 @@ bool OmxDecoder::SetAudioFormat() {
>   return true;
> }
> 
> void OmxDecoder::ReleaseDecoder()
> {
>   mDecoder = nullptr;
> }
> 
>-bool OmxDecoder::NotifyDataArrived(const char* aBuffer, uint32_t aLength, int64_t aOffset)
>-{
>-  if (!mAudioTrack.get() || !mIsMp3 || !mMP3FrameParser.IsMP3() || !mDecoder) {
>-    return false;
>-  }
>-
>-  mMP3FrameParser.Parse(aBuffer, aLength, aOffset);
>-
>-  int64_t durationUs = mMP3FrameParser.GetDuration();
>-
>-  if (durationUs != mDurationUs) {
>-    mDurationUs = durationUs;
>-
>-    MOZ_ASSERT(mDecoder);
>-    ReentrantMonitorAutoEnter mon(mDecoder->GetReentrantMonitor());
>-    mDecoder->UpdateEstimatedMediaDuration(mDurationUs);
>-  }
>-
>-  return true;
>-}
>-
> void OmxDecoder::ReleaseVideoBuffer() {
>   if (mVideoBuffer) {
>     mVideoBuffer->release();
>     mVideoBuffer = nullptr;
>   }
> }
> 
> void OmxDecoder::ReleaseAudioBuffer() {
>@@ -1080,51 +893,8 @@ void OmxDecoder::RecycleCallbackImp(Text
> }
> 
> /* static */ void
> OmxDecoder::RecycleCallback(TextureClient* aClient, void* aClosure)
> {
>   OmxDecoder* decoder = static_cast<OmxDecoder*>(aClosure);
>   decoder->RecycleCallbackImp(aClient);
> }
>-
>-int64_t OmxDecoder::ProcessCachedData(int64_t aOffset, bool aWaitForCompletion)
>-{
>-  // We read data in chunks of 32 KiB. We can reduce this
>-  // value if media, such as sdcards, is too slow.
>-  // Because of SD card's slowness, need to keep sReadSize to small size.
>-  // See Bug 914870.
>-  static const int64_t sReadSize = 32 * 1024;
>-
>-  NS_ASSERTION(!NS_IsMainThread(), "Should not be on main thread.");
>-
>-  MOZ_ASSERT(mResource);
>-
>-  int64_t resourceLength = mResource->GetCachedDataEnd(0);
>-  NS_ENSURE_TRUE(resourceLength >= 0, -1);
>-
>-  if (aOffset >= resourceLength) {
>-    return 0; // Cache is empty, nothing to do
>-  }
>-
>-  int64_t bufferLength = std::min<int64_t>(resourceLength-aOffset, sReadSize);
>-
>-  nsAutoArrayPtr<char> buffer(new char[bufferLength]);
>-
>-  nsresult rv = mResource->ReadFromCache(buffer.get(), aOffset, bufferLength);
>-  NS_ENSURE_SUCCESS(rv, -1);
>-
>-  nsRefPtr<OmxDecoderNotifyDataArrivedRunnable> runnable(
>-    new OmxDecoderNotifyDataArrivedRunnable(this,
>-                                            buffer.forget(),
>-                                            bufferLength,
>-                                            aOffset,
>-                                            resourceLength));
>-
>-  rv = NS_DispatchToMainThread(runnable.get());
>-  NS_ENSURE_SUCCESS(rv, -1);
>-
>-  if (aWaitForCompletion) {
>-    runnable->WaitForCompletion();
>-  }
>-
>-  return resourceLength - aOffset - bufferLength;
>-}
>diff --git a/content/media/omx/OmxDecoder.h b/content/media/omx/OmxDecoder.h
>--- a/content/media/omx/OmxDecoder.h
>+++ b/content/media/omx/OmxDecoder.h
>@@ -157,18 +157,16 @@ public:
>   bool IsWaitingMediaResources();
>   bool AllocateMediaResources();
>   void ReleaseMediaResources();
>   bool SetVideoFormat();
>   bool SetAudioFormat();
> 
>   void ReleaseDecoder();
> 
>-  bool NotifyDataArrived(const char* aBuffer, uint32_t aLength, int64_t aOffset);
>-
>   void GetDuration(int64_t *durationUs) {
>     *durationUs = mDurationUs;
>   }
> 
>   void GetVideoParameters(int32_t* aDisplayWidth, int32_t* aDisplayHeight,
>                           int32_t* aWidth, int32_t* aHeight) {
>     *aDisplayWidth = mDisplayWidth;
>     *aDisplayHeight = mDisplayHeight;

Comment 41

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Attachment: bug.patch

Change the title.

Comment 42

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Attachment: test_playback.html.patch

apply jwwang suggestion.

Comment 43

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8486926 [details] [diff] [review]
bug.patch

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

::: content/media/omx/MediaOmxReader.cpp
@@ +45,5 @@
> +
> +  void Run()
> +  {
> +    MOZ_ASSERT(!NS_IsMainThread());
> +    MOZ_ASSERT(mOmxReader.get());

We would like to catch errors as early as possible. Therefore, you should assert mOmxReader in the constructor.

@@ +84,5 @@
> +    mCompletedMonitor("OmxReaderNotifyDataArrived.mCompleted"),
> +    mCompleted(false)
> +  {
> +    MOZ_ASSERT(mOmxReader.get());
> +    MOZ_ASSERT(mBuffer.get() || !mLength);

Check |mBuffer.get() && mLength|. It only makes sense when mBuffer is not null and mLength > 0.

Furthermore, you should also assert the followings:
aBuffer != nullptr
aLength > 0
aOffset >= 0 && aOffset < aFullLength
aOffset + aLength <= aFullLength

@@ +144,5 @@
> +  nsRefPtr<MediaOmxReader> mOmxReader;
> +  nsAutoArrayPtr<const char>       mBuffer;
> +  uint64_t                         mLength;
> +  int64_t                          mOffset;
> +  uint64_t                         mFullLength;

Declare mLength and mFullLength as int64_t. Mixing Signed/unsigned cam lead to subtle issues.

@@ +568,5 @@
>    DebugOnly<nsresult> result = mOmxDecoder->Play();
>    NS_ASSERTION(result == NS_OK, "OmxDecoder should be in play state to continue decoding");
>  }
>  
> +int64_t MediaOmxReader::ProcessCachedData(int64_t aOffset, bool aWaitForCompletion)

Please document the meaning of the return value. I am having a hard time understanding what it means when return value is 0, -1 or >0 respectively.

@@ +601,5 @@
> +                                           bufferLength,
> +                                           aOffset,
> +                                           resourceLength));
> +
> +  rv = NS_DispatchToMainThread(runnable.get(), NS_DISPATCH_SYNC);

You should only pass NS_DISPATCH_SYNC when aWaitForCompletion is true.

@@ +604,5 @@
> +
> +  rv = NS_DispatchToMainThread(runnable.get(), NS_DISPATCH_SYNC);
> +  NS_ENSURE_SUCCESS(rv, -1);
> +
> +  if (aWaitForCompletion) {

You don't need to check |aWaitForCompletion| for NS_DISPATCH_SYNC will guarantee the runnable is completed when we reach here.

Comment 44

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Attachment: bug-v2.patch

Hi Edwin, 
Do you have time to review this one? Thanks.
try result:
https://tbpl.mozilla.org/?tree=Try&rev=eb6fd79563a2

Comment 45

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Comment on attachment 8488336 [details] [diff] [review]
bug-v2.patch

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

LGTM. Looks like JW caught everything before I could get to it. :)

Comment 46

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Attachment: check-in patch 1

check-in patch 1.
try result:
https://tbpl.mozilla.org/?tree=Try&rev=ea5e333ef30d

Comment 47

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Attachment: check-in patch 2

Check-in patch 2, enable mp3 duration check test case on b2g.

Comment 48

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/92a7c3a6a88a
https://hg.mozilla.org/integration/mozilla-inbound/rev/54f2c7998c62

Comment 49

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/92a7c3a6a88a
https://hg.mozilla.org/mozilla-central/rev/54f2c7998c62