1515658 - Incorrect audio duration is displayed for a blob audio composed of blob slices

Status: VERIFIED FIXED

(Core :: Audio/Video: Playback, defect, P3)

Description

[Emil Ghitta, QA [:emilghitta]]

[Affected versions]:
Firefox 60.4.0esr (BuildId:20181203164059).
Firefox 66.0a1 (BuildId:20181219220049).

[Unaffected versions]: Due to bug 1514581
Firefox 64.0 (BuildId:20181206201918).
Firefox 65.0b5 (BuildId:20181217180946).

[Affected platforms]:
Windows 10 64bit
macOS 10.14.
Ubuntu 16.04 64bit.

[Steps to reproduce]:
1. Open https://jsfiddle.net/tzyjr059/
2. Select a file of audio/wav type. For example, 440Hz 0:05.00 wav found in https://www.mediacollege.com/audio/tone/download/

[Expected result]:
The correct audio duration is displayed:
0:00 / 0:05

[Actual result]:
The following audio duration is displayed:
13:31:36 / 13:31:36

[Regression range]:
This seems to be a regression:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=e897e367d3bd489422d86fbdfac54925c18329d2&tochange=3d918ff5d63442d7b88e1b7e9cb03b832bc28fdf

I didn't managed to generate a more "precise" pushlog since mozregression keeps skipping builds.

Comment 1

[Andrea Marchesini [:baku]]

jya, where do we calculate the media size? Could it be that we try to read data synchronously, nsIAsyncInputStream doesn't allow this and we show some default value?

Comment 2

[Liz Henry (:lizzard) (relman/hg->git project)]

Since this is triaged and has a priority set, marking this fix-optional to remove it from regression triage. 
Happy to still take a patch in nightly.

Comment 3

[Jean-Yves Avenard [:jya]]

(In reply to Andrea Marchesini [:baku] from comment #1)
> jya, where do we calculate the media size? Could it be that we try to read
> data synchronously, nsIAsyncInputStream doesn't allow this and we show some
> default value?

This is done via MediaResource::GetLength which is a synchronous API.

How the length is determine by the underlying medium varies, for http requests it's a combination of Content-Length headers and how much we've read so far.

GetLength is supposed to return -1 if the size is unknown, most of the media code then will use the current position as duration or until the last sample is demuxed

Comment 4

[Andrea Marchesini [:baku]]

Attachment: blob_media.patch

We know the blobURL's size. We can use it directly for this particular corner case.

Comment 5

[Jean-Yves Avenard [:jya]]

Comment on attachment 9034180 [details] [diff] [review]
blob_media.patch

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

r=me with comments addressed.

::: dom/media/ChannelMediaResource.h
@@ +252,5 @@
>    MediaCacheStream mCacheStream;
>  
>    ChannelSuspendAgent mSuspendAgent;
> +
> +  // The size of the stream if known.

Should indicate further.
something like:

// The size of the stream if known at construction time (such as with blob)

as no doubt the next person will wonder why we have so many exceptions related to length

@@ +253,5 @@
>  
>    ChannelSuspendAgent mSuspendAgent;
> +
> +  // The size of the stream if known.
> +  int64_t mKnownStreamLength;

this name is confusing IMHO. This is indicate if we know the length at construction time.
We already have mFirstReadLength as a workaround for the first length reported by the channel.

Can't think of a better name unfortunately. In any case it should be const too.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c9f8232272e
ChannelMediaResource should use the BlobURLs' length when known, r=jya

Comment 7

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/9c9f8232272e

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Please request Beta approval on this when you get a chance.

Comment 9

[Andrea Marchesini [:baku]]

Comment on attachment 9034180 [details] [diff] [review]
blob_media.patch

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1514581

User impact if declined: Wrong media data length for real OS files when used as blobURLs

Is this code covered by automated tests?: No

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: Yes

If yes, steps to reproduce: See the bug description

List of other uplifts needed: none

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): The fix is about using the blob size instead of using -1. It's just about propagating an existing information.

String changes made/needed: none

Comment 10

[Emil Ghitta, QA [:emilghitta]]

This issue is verified fixed using Firefox 66.0a1 (BuildId:20190106214444) on Windows 10 64bit, macOS 10.13.6 and Ubuntu 16.04 64bit.

Leaving the qe-verify+ flag until this gets verified in beta as well.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9034180 [details] [diff] [review]
blob_media.patch

[Triage Comment]
Fixes a regression caused by bug 1514581 which caused the wrong media data length to be reported in some cases. Approved for 65.0b9.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/47a7517154c0

Comment 13

[Emil Ghitta, QA [:emilghitta]]

This is verified fixed using Firefox 65.0b9 (BuildId:20190107180200) on Windows 10 64bit, macOS 10.13.6 and Ubuntu 16.04 64bit.