975928 - GStreamer backend reports buffered ranges end time outside of reported duration for mp3

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

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

In GStreamerReader::QueryDuration(), there's this block:

{
 ReentrantMonitorAutoEnter mon(mDecoder->GetReentrantMonitor());
 int64_t media_duration = mDecoder->GetMediaDuration();
 if (media_duration != -1 && media_duration > duration) {
   // We decoded more than the reported duration (which could be estimated)
   LOG(PR_LOG_DEBUG, ("decoded duration > estimated duration"));
   duration = media_duration;
 }
}

That should be "duration > media_duration" I think.

This manifests as a failure in test_buffered.html:

ERROR TEST-UNEXPECTED-FAIL | /tests/content/media/test/test_buffered.html | owl.mp3: First range end should be media end - got 3.343673, expected 3.343616

This failure happens on owl.mp3. The duration estimated by the MP3FrameParser is 3.343616, but GStreamer decodes through the file(or for some other reason) and decides that the duration is 3.343673 and then runs the block above, which is supposed to update the duration reported to the reader, but doesn't because the code is wrong.

Comment 1

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

Attachment: Patch: fix logic

Invert logic, so that duration changes are propagated to MediaDecoder.

Comment 2

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

Attachment: Patch 2: Make test_buffered.html deterministic

This patch changes test_buffered.html to load the resource using XHR, rather than our normal mediacache route. This means we can be confident that the resource is loaded before beginning the test. We no longer need to play back the media to the end and hope that the media cache was in a good mood and didn't evict any data.

Comment 3

[Matthew Gregan [:kinetik]]

Comment on attachment 8380443 [details] [diff] [review]
Patch 2: Make test_buffered.html deterministic

Looks fine, but please convert all of the newly added tabs to spaces before checking in.

Comment 4

[Alessandro Decina]

(In reply to Chris Pearce (:cpearce) from comment #0)
> In GStreamerReader::QueryDuration(), there's this block:
> 
> {
>  ReentrantMonitorAutoEnter mon(mDecoder->GetReentrantMonitor());
>  int64_t media_duration = mDecoder->GetMediaDuration();
>  if (media_duration != -1 && media_duration > duration) {
>    // We decoded more than the reported duration (which could be estimated)
>    LOG(PR_LOG_DEBUG, ("decoded duration > estimated duration"));
>    duration = media_duration;
>  }
> }
> 
> That should be "duration > media_duration" I think.
> 
> This manifests as a failure in test_buffered.html:
> 
> ERROR TEST-UNEXPECTED-FAIL | /tests/content/media/test/test_buffered.html |
> owl.mp3: First range end should be media end - got 3.343673, expected
> 3.343616
> 
> This failure happens on owl.mp3. The duration estimated by the
> MP3FrameParser is 3.343616, but GStreamer decodes through the file(or for
> some other reason) and decides that the duration is 3.343673 and then runs
> the block above, which is supposed to update the duration reported to the
> reader, but doesn't because the code is wrong.

It's not clear to me where MP3Parser fits in the picture.

The code is (was) correct according to the logic explained in the comment in the source, which is: 

if mDecoder->GetMediaDuration() is > gst_element_query_duration(), considering that mDecoder->GetMediaDuration() (at least when the code was written) returned the effective duration of the (so far) decoded data, and that instead gst_element_query_duration() returned an estimate, if the effective duration > estimated duration => effective beats estimated.

Comment 5

[Alessandro Decina]

(In reply to Chris Pearce (:cpearce) from comment #0)

> ERROR TEST-UNEXPECTED-FAIL | /tests/content/media/test/test_buffered.html |
> owl.mp3: First range end should be media end - got 3.343673, expected
> 3.343616

Also, while I admit I haven't looked closely into this, just looking at this line two things come immediately to mind:

- in this test (and others) we're probably checking times with a too high resolution
- i'm not sure why we think that 3.343616 is the correct length. Decoding the file with two different decoders (libav and mad), I get a duration of 

3.369795918 (mad)
3.3697957929999998 (libav)

so if anything it should be 3.36979 ?

Comment 6

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

(In reply to Alessandro Decina from comment #4)
> It's not clear to me where MP3Parser fits in the picture.

We use MP3FrameParser to estimate the duration of MP3 files, as on some platforms (Android, MacOSX, Windows/DirectShow) the duration estimate the platform returns is badly wrong for variable bitrate files. We use MP3FrameParser everywhere (including in the GStreamer backend where the MP3 duration estimate is OK I understand) to ensure that we get consistent behaviour across platforms.

 
> The code is (was) correct according to the logic explained in the comment in
> the source, which is: 
> 
> if mDecoder->GetMediaDuration() is > gst_element_query_duration(),
> considering that mDecoder->GetMediaDuration() (at least when the code was
> written) returned the effective duration of the (so far) decoded data, and
> that instead gst_element_query_duration() returned an estimate, if the
> effective duration > estimated duration => effective beats estimated.

Ah, I didn't realise that gst_element_query_duration() was an estimate. I guess we should remove QueryDuration() and always use the MediaDecoder's duration then, so that we only expose buffered ranges with a consistent duration/end time to JavaScript. We're only using the duration returned by QueryDuration() when the resource is fully buffered.



(In reply to Alessandro Decina from comment #5)
> (In reply to Chris Pearce (:cpearce) from comment #0)
> 
> > ERROR TEST-UNEXPECTED-FAIL | /tests/content/media/test/test_buffered.html |
> > owl.mp3: First range end should be media end - got 3.343673, expected
> > 3.343616
> 
> Also, while I admit I haven't looked closely into this, just looking at this
> line two things come immediately to mind:
> 
> - in this test (and others) we're probably checking times with a too high
> resolution
> - i'm not sure why we think that 3.343616 is the correct length. Decoding
> the file with two different decoders (libav and mad), I get a duration of 
> 
> 3.369795918 (mad)
> 3.3697957929999998 (libav)
 
DirectShow decodes owl.mp3 to a duration of 3.369795s.

> so if anything it should be 3.36979 ?

Yeah probably.

Edwin: Is MP3FrameParser's duration estimate is wrong here?

Comment 7

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

(In reply to Chris Pearce (:cpearce) from comment #6)
> > 3.369795918 (mad)
> > 3.3697957929999998 (libav)
>  
> DirectShow decodes owl.mp3 to a duration of 3.369795s.
> 
> > so if anything it should be 3.36979 ?
> 
> Yeah probably.
> 
> Edwin: Is MP3FrameParser's duration estimate is wrong here?

Weird. I get 3.343s from both mediainfo and libav v0.8.9.

Comment 8

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

Attachment: Patch: Always use MediaDecoder duration when fully downloaded

OK, let's just use the MediaDecoders duration, since that is what we're reporting in HTMLMediaElement.duration, so this way HTMLMediaElement.buffered.end(0) will always match up to HTMLMediaElement.duration if we've fully downloaded the media.

The issue with the MP3FrameParser getting a different (possibly/probably incorrect) duration than what we get with some other decoders we should investigate. But irrespective of whether the duration that MP3FrameParser is getting is incorrect or not, we should be using the duration reported by HTMLMediaElement.duration here.

Comment 9

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/59f6a07a619a
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ad7988d53c9

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/59f6a07a619a
https://hg.mozilla.org/mozilla-central/rev/0ad7988d53c9