Closed Bug 1515658 Opened 1 year ago Closed 1 year ago

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

Categories

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

defect

Tracking

()

VERIFIED FIXED
mozilla66
Tracking Status
firefox-esr60 --- wontfix
firefox64 --- wontfix
firefox65 --- verified
firefox66 --- verified

People

(Reporter: emilghitta, Assigned: baku)

References

Details

(Keywords: regression)

Attachments

(1 file)

[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.
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?
Flags: needinfo?(jyavenard)
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.
(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
Flags: needinfo?(jyavenard)
Attached patch blob_media.patchSplinter Review
We know the blobURL's size. We can use it directly for this particular corner case.
Assignee: nobody → amarchesini
Attachment #9034180 - Flags: review?(jyavenard)
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.
Attachment #9034180 - Flags: review?(jyavenard) → review+
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c9f8232272e
ChannelMediaResource should use the BlobURLs' length when known, r=jya
https://hg.mozilla.org/mozilla-central/rev/9c9f8232272e
Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
Please request Beta approval on this when you get a chance.
Flags: qe-verify+
Flags: needinfo?(amarchesini)
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
Flags: needinfo?(amarchesini)
Attachment #9034180 - Flags: approval-mozilla-beta?
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.
Status: RESOLVED → VERIFIED
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.
Attachment #9034180 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

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

Flags: qe-verify+
Has Regression Range: --- → yes
You need to log in before you can comment on or make changes to this bug.