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

Status: VERIFIED FIXED

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

Description

[KichikuouChrome]

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.

Comment 1

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

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.

Comment 2

[Andrea Marchesini [:baku]]

Attachment: async_streams.patch

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).

Comment 4

[Jean-Yves Avenard [:jya]]

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?

Comment 5

[Andrea Marchesini [:baku]]

> 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.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a3885c792aa5
Don't use async streams with FileMediaResource, r=jya

Comment 7

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/a3885c792aa5

Comment 8

[Andreas Pehrson [:pehrsons]]

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 9

[Andrea Marchesini [:baku]]

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:

Comment 10

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/542014a5898e

Comment 12

[Emil Ghitta, QA [:emilghitta]]

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.