Closed Bug 1514581 Opened 8 months ago Closed 8 months ago

<audio> fails to play a blob composed of blob slices

Categories

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

65 Branch
defect

Tracking

()

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

People

(Reporter: KichikuouChrome, Assigned: baku)

References

Details

(Keywords: regression)

Attachments

(1 file)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.14; rv:65.0) Gecko/20100101 Firefox/65.0

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/
3. Hit the play button of the audio element.



Actual results:

The audio isn't played. Console says:

Error Code: NS_ERROR_DOM_MEDIA_METADATA_ERR (0x806e0006)



Expected results:

The audio should be played. This page splits the audio file into two blob slices and simply joins them, so it should be played just like the original sound.
Thanks for the report.

This looks like a regression. Appears to be as a result of Bug 1460561. :baku, does this seem like the kind of thing that patch could have impacted?

Provided fiddle could be adapted to test case for when we fix.
Status: UNCONFIRMED → NEW
Has Regression Range: --- → yes
Has STR: --- → yes
Ever confirmed: true
Flags: needinfo?(amarchesini)
Priority: -- → P2
The problem here is that we are using FileMediaResource with an async inputStream. FileMediaResource uses ::Read(), and the stream returns NS_BASE_STREAM_WOULD_BLOCK error.

We cannot relay on QI nsIAsyncInputStream because there are streams implementing that interface but able to work synchronously as well (for instance anything wrapped in a NonBlockingAsyncInputStream class).

The previous hack was checking nsISeekableStream interface. Which was OK for a while, but now, nsMultiplexInputStream wraps any stream in a nsBufferedInputStream (see bug 1460561) and now, nsMultiplexInputStream is always seekable.

This patch checks if the stream knows its own length synchronously. If it does, we can be sure that the reading is synchronous too and we can use FileMediaResource.

As usual, the fallback is to use CloneableWithRangeMediaResource().

This issue can be reproduced only with Blobs created from OS files (via file picker or Entries API).
Assignee: nobody → amarchesini
Flags: needinfo?(amarchesini)
Attachment #9032116 - Flags: review?(jyavenard)
Duplicate of this bug: 1499560
Comment on attachment 9032116 [details] [diff] [review]
async_streams.patch

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

LGTM

::: dom/media/BaseMediaResource.cpp
@@ +64,5 @@
> +    // FileMediaResource. If the size is known, it means that the reading
> +    // doesn't require any async operation.  This is required because
> +    // FileMediaResource doesn't work with nsIAsyncInputStreams.
> +    int64_t length;
> +    if (InputStreamLengthHelper::GetSyncLength(stream, &length) &&

What about streams that knows their size (such as nsHttp ones) but the size may change over time.
Couldn't this triggered false positive?
Attachment #9032116 - Flags: review?(jyavenard) → review+
> What about streams that knows their size (such as nsHttp ones) but the size
> may change over time.
> Couldn't this triggered false positive?

They are not BlobURLs and they are handled by ChannelMediaResource. By spec, Blobs cannot change the size, but, in reality they do in case they are built on top of underlying OS files. What to do in this case, is an open question. We should fix that at a spec level.
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a3885c792aa5
Don't use async streams with FileMediaResource, r=jya
https://hg.mozilla.org/mozilla-central/rev/a3885c792aa5
Status: NEW → RESOLVED
Closed: 8 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
Andrea, what do you think of uplifting this to 65?
I'd love to see it uplifted so the MediaRecorder regression can be fixed as soon as possible.
Flags: needinfo?(amarchesini)
Comment on attachment 9032116 [details] [diff] [review]
async_streams.patch

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1460561

User impact if declined: Sliced Blobs pointing to real files, cannot be used as MediaSource.

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): We use a different heuristic to determine if a blob's inputStream is asynchronous or not. This is similar to what we do in necko and elsewhere, and it's based on InputSteamLengthHelper class.

String changes made/needed:
Flags: needinfo?(amarchesini)
Attachment #9032116 - Flags: approval-mozilla-beta?
Blocks: 1460561
Flags: qe-verify+
Comment on attachment 9032116 [details] [diff] [review]
async_streams.patch

[Triage Comment]
Fixes media playback issues. Thanks for landing a test in the other bug too. Approved for 65.0b6.
Attachment #9032116 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Depends on: 1515658
I have managed to reproduce the issue described in comment 0 using Firefox 65.0b5.

This issue is verified fixed using Firefox 66.0a1 (BuildId:20181219220049) and Firefox 65.0b6 (provided in comment 11) on Windows 10 64bit, macOS 10.9 and Ubuntu 16.04 64bit.

The audio is successfully played back.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.