Closed Bug 1514581 Opened 8 months ago Closed 8 months ago
<audio> fails to play a blob composed of blob slices
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.
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
Attachment #9032116 - Flags: review?(jyavenard)
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 email@example.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/a3885c792aa5 Don't use async streams with FileMediaResource, r=jya
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.
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:
Attachment #9032116 - Flags: approval-mozilla-beta?
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+
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.
You need to log in before you can comment on or make changes to this bug.