1646719 - Canvas WPT test using preload=auto flaky-fails in Firefox

Status: RESOLVED FIXED

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

Description

[smcgruer]

User Agent: Mozilla/5.0 (X11; CrOS x86_64 13020.67.0) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/83.0.4103.97 Safari/537.36

Steps to reproduce:

See https://crbug.com/1096057 for the Chromium bug. After investigation, it looks like the flaky-failure is a problem in Firefox rather than the test (with apologies if its not!), so filing this bug.

The GitHub PR is https://github.com/web-platform-tests/wpt/pull/23998, which added a video.preload = "auto"; to a few locations in html/canvas/resources/canvas-tests.js . This caused the Firefox Nightly stability checks on WPT to start failing, with a flaky timeout on two tests:

• html/canvas/element/manual/imagebitmap/createImageBitmap-origin.sub.html - 'redirected to cross-origin HTMLVideoElement: origin unclear bitmaprenderer.transferFromImageBitmap'
• html/semantics/embedded-content/the-canvas-element/security.pattern.fillStyle.sub.html - 'redirected to cross-origin HTMLVideoElement: Setting fillStyle to an origin-unclear pattern makes the canvas origin-unclean'

Some perhaps relevant info from the Chromium bug:

• "Test just waits for canplaythrough event to occur and the video clip is only ~2mb. So I guess canplaythough isn't happening or is delayed for some reason? Waiting for canplaythrough is a common pattern, so I'm surprised there would be an issue here."
• "preload=auto should be the default when unset, Chrome uses unset to mean preload=metadata though, maybe Firefox is also doing something specific."
• "It is a 300s media clip, so maybe there are cases where Firefox is detecting canplaythrough as not being possible due to the length of the clip relative to how fast it was downloaded before it reaches the end of loading w/o playback. Still seems like a firefox bug though."

Comment 1

[Jeronimo Torti (Please send NI request to Brindusa Tot)]

Hi,
I don't have the technical knowledge to confirm this issue but I'll add product and component so the team can make some research on it. Hopefully, someone with a more deep understanding of this matter can help. Feel free to route this ticket to the corresponding team.

Regards,
Jerónimo.

Comment 2

[Alastor Wu [:alwu]]

I can confirm that those test can both pass if we change to listen to canplay, not canplaythrough.
I'll check what happened.

Comment 3

[C.M.Chang[:chunmin] | PTO]

I found the test can pass if media.cache_size is large than 5307 (media.cache_size >= 5308). The media.cache_size used in the test is 1000 by default.

However, I am not able to reproduce it without web-platform-test framework, even I adjust the media.cache_size to 1000. Not sure why. I would need to take a look at the code.

Comment 4

[C.M.Chang[:chunmin] | PTO]

Attachment: minimal-reproduce-with-extra-log.diff

Comment 5

[C.M.Chang[:chunmin] | PTO]

Attachment: MediaCache-and-MediaElement.moz_log

The log is created by running MOZ_LOG="MediaCache:5, nsMediaElement:5" MOZ_LOG_FILE=<FILENAME> ./mach wpt html/canvas/element/manual/imagebitmap/createImageBitmap-origin.sub.html.

The reason why the test is time out:

In brief, from the line 1276, when the sixth stream is opened, the cache is full [1], and all of the cache streams are alive(non-closed) but got suspended. The worst part is that the MediaCache::Update() gets stuck (all of the stream actions(StreamAction) are NONE). No video is played in the test.

In that case ,it's better to find a way to call ChannelMediaResource::CacheClientNotifySuspendedStatusChanged from MediaCache so the HTMLMediaElement::mDownloadSuspendedByCache can be set and so HTMLMediaElement can fire a canplaythrough event once the first frame is loaded.

[1] Since the createImageBitmap-origin.sub.html use preload=auto, so we would cache as much as data we can

Comment 6

[C.M.Chang[:chunmin] | PTO]

Attachment: potential-fix.patch

This patch queues a suspended-status-update fwhen a new cloned (and suspended) stream is opened and the cache got stuck. Try server result is for mochitest-media is here.

Comment 7

[C.M.Chang[:chunmin] | PTO]

Attachment: potential-fix2.patch

Another approach: Inform the streams that never get their suspended-status-change before about they are suspended, in MediaCache::Update. The test result is here

Comment 8

[C.M.Chang[:chunmin] | PTO]

Attachment: Bug 1646719 - P1: Add more debugging logs

Add some logs that can help us to check how many cache-streams are
suspended and the working status in MediaCache::Update

Comment 9

[C.M.Chang[:chunmin] | PTO]

Attachment: Bug 1646719 - P2: Queue a suspended status check when cloning a cache-stream

If a cloned cache-stream is added to a cache that currently gets stuck,
the stream will keep being suspended until the cache starts working
again. However, the cache can keep being immobile forever if none of its
cache-stream is working. In this case, we should inform the
media-element paired with this stream that it's suspended by cache. This
will force the media-element transit its ready-state to
HAVE_ENOUGH_DATA.

A cloned stream is suspended by default. If it's added to an immobile
cache, then it's paired media-element will never have enough data to
fire a "canplaythrough" event. In this case, we need to force the
media-element fire the event and pray the event can make the
media-element starts playing and eventually revitalize the cache.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1646719#c5

Depends on D92124

Comment 10

[Jean-Yves Avenard [:jya]]

(In reply to C.M.Chang[:chunmin] from comment #9)

Created attachment 9179091 [details]
Bug 1646719 - P2: Send suspended status when adding a cloned stream to a motionless cache

If a cloned cache-stream is added to a cache that currently gets stuck,
the stream will keep being suspended until the cache starts working
again. However, the cache can keep being immobile forever if none of its
cache-stream is working. In this case, we should inform the
media-element paired with this stream that it's suspended by cache. This
will force the media-element transit its ready-state to
HAVE_ENOUGH_DATA.

A cloned stream is suspended by default. If it's added to an immobile
cache, then it's paired media-element will never have enough data to
fire a "canplaythrough" event. In this case, we need to force the
media-element fire the event and pray the event can make the
media-element starts playing and eventually revitalize the cache.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1646719#c5

Depends on D92124

I'm confused by this comment.

The way I read it, caching is blocked, we suspended more caching to occur and we fake the move of readyState when in fact it doesn't happen. That seems to just be a workaround to a logic error in the cache and bypassing a more serious bug.

Comment 11

[C.M.Chang[:chunmin] | PTO]

(In reply to Jean-Yves Avenard [:jya] from comment #10)

Depends on D92124

I'm confused by this comment.

The way I read it, caching is blocked, we suspended more caching to occur and we fake the move of readyState when in fact it doesn't happen. That seems to just be a workaround to a logic error in the cache and bypassing a more serious bug.

The forced transition exists for ages. In fact, this force transition occurs everytime once a cache-stream gets suspended, as long as the first-frame of the video is loaded. In this case, this force transition occurs when loading the first cache-stream (line 75-79, 171 in attachment 9178823 [details]), without applying the patch. The web page asks to use video.preload="auto" (wpt #23998) so media-cache tries to download as much data as it can. But the cache size is too small. The cache-size is only 1000 KB while the video size is 2.3 MB. The forced transition works for years without applying the patch.

Bug 755533 tries to solve the same problem. The force transition is first made in that bug. I guess this is an edge case it leaves. This patch only affects the cloned stream, which is cloned from a stream that is downloading data and sharing the data with it. The force transition only take place after the first frame is loaded. So the "canplaythrough" won't be fired when there is no frame to play. It has at least the first frame. What this patch tries to do is to align with the policy that already exists, when the cache gets stuck.

Aside form that, it doesn't break the spec for video.preload="auto":

Hints to the user agent that the user agent can put the user's needs first without risk to the server, up to and including optimistically downloading the entire resource.

Comment 12

[C.M.Chang[:chunmin] | PTO]

Attachment: Bug 1646719 - P2: Queue a suspended status check when cloning a cache-stream

• What this patch does:

Send a "download-suspended-by-cache" signal to the media-element paired
with a cloned cache-stream right after the cache-stream is cloned. That
eventually make the media-element fires a "canplaythrough" event.

• Why this patch is needed:

It solves a WPT timeout issue. This WPT test waits a "canplaythrough"
event.

• What problem this patch solves is:

This patch addresses the problem mentioned in [1].

Each media-element pairs with a cache-stream downloading the data it
needs. When the data-download of a cache-stream gets suspended, the
media-element paired with the cache-stream should be notified. This
notification is one of the factor to move the ready-state of the
media-element to HAVE_ENOUGH_DATA. However, the media-element paired
with a cloned cache-stream may never receive this notification. And the
worst is that it may never have a chance to download enough data it
needs to move the ready-state to HAVE_ENOUGH_DATA in some cases. The
"canplaythrough" event is fired when the ready-state is transited to
HAVE_ENOUGH_DATA, so it will never be dispatched in this case. This can
happen when a media-element paired with a cloned cache-stream is created
after the cache is full, and all of the cache-streams are suspended.
(Cloned cache-stream is a cache-stream cloned from another cache-stream
that shares the same underlying data with it since their paired
media-elements have the same src (of HTMLMediaElement)).

In usual case, if the cache-stream gets suspended from non-suspended, it
will send a "download-suspended-by-cache=true" signal to its paired
media-element when running MediaCache::Update(). In fact, all other
cache-streams sharing the same underlying data will send this signal at
the same time if necessary. (Later, once the cache-stream is resumed
from suspended to non-suspended it will send a
"download-suspended-by-cache=false" signal to its paired media-element.
All other cache-streams sharing the same underlying data will do the
same if necessary.) The media-element keeps tracking that signal it
receives. After the first-frame of the media-element is loaded, the
ready-state of the media-element will be transited to HAVE_ENOUGH_DATA
by force if the signal is true. (Otherwise, the ready-state will be
inferred by other information.)

When cloning a cache-stream from another one, the cloned cache-stream is
suspended by default. If it's added to a jammed cache that all of the
cache-streams are suspended since the cache is full, then it never has a
chance to fire the "download-suspended-by-cache" signal. Both its
source-stream and itself have no status-change between suspended and
non-suspended, so MediaCache::Update is unable to send the signal.

In this case, we should force the media-element paired with the newly
cloned cache-stream transits its ready-state to HAVE_ENOUGH_DATA, which
follows the existing mechanism, by queueing a status-change-update when
cloning the stream. The status-change-update will be run in the
MediaCache::Update and it will check what the signal should be sent.
Once the "download-suspended-by-cache=true" is sent, the
"canplaythrough" event of the media-element will be dispatched (after
its first-frame is loaded), which can possibly make the media-element
starts playing and lead to a cache-seek that can revitalize the cache
eventually.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1646719#c5

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/76816622157c
P1: Add more debugging logs r=gerald
https://hg.mozilla.org/integration/autoland/rev/c845576c6a34
P2: Queue a suspended status check when cloning a cache-stream r=jya

Comment 14

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/76816622157c
https://hg.mozilla.org/mozilla-central/rev/c845576c6a34