New issue
Advanced search Search tips

Issue 633591 link

Starred by 3 users

Issue metadata

Status: Available
Owner: ----
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux
Pri: 2
Type: Bug



Sign in to add a comment

Setting src asynchronously of calling play() on a media with preload none doesn't play

Project Member Reported by mlamouri@chromium.org, Aug 2 2016

Issue description

Test page: https://mounirlamouri.github.io/sandbox/media/set-src-timeout-no-preload.html

STR:
- load test page

Expected result: video plays
Actual result: video does not play

Additional information:
- works on Firefox.
- the controls are in a broken state.
 
Labels: Needs-BlinkMediaTriage
Labels: -Needs-BlinkMediaTriage
Hi, 

  I just reproduced this issue. As per the W3C spec, the moment play() is called the autoplay option (if it exists is set to false). However, I am not sure if it is correct to expect that setting src on a video object which has had play() called on it before , should actually play the changed src.

I can investigated further and try to isolate a fix , if the behavior as outlined in the test case is actually valid.
Updating this after testing on browserstack:
- Edge (15): works (like Firefox)
- Safari (10.1): does not work (like Chrome)

Most likely, this is a WebKit bug that stayed in Blink.
Actually, when I say "a bug", I meant a different behaviour. As comment #3 mentioned, it's unclear whether Chrome/Safari or Firefox/Edge are correct here.
Hi, 

 Thanks for getting back to me and clarifying. 
I have added a short patch which gets the behavior as seen in Firefox here:
https://codereview.chromium.org/2910683003/

 Do let me know your thoughts.

Thanks,
George.

I couldn't find a clear answer from the specification. However, in Blink, this seems to happen:
- playback starts with 'play' and 'waiting' events firing
- setIgnorePreloadNone is called
- src attribute is parsed
- ignore_preload_none_ is reversed back to false
- 'loadstart' event is fired
- load algorithm is running
- load does not happen because preload is set to 'none'
- loading is suspended and 'suspend' event is fired

What might be interesting to understand is why `paused_` is never set back to true.

Comment 8 by foolip@chromium.org, May 30 2017

Cc: sriram...@samsung.com
Right before the src attribute is set, paused is false and networkState is 0. Correctly per https://html.spec.whatwg.org/multipage/embedded-content.html#media-element-load-algorithm, paused remains false afterwards. I don't know of anything in the spec that would explain why it doesn't play, so this looks like a bug in Blink/WebKit.

The way that ignore_preload_none_ is maintained is suspect, and the TODO before HTMLMediaElement::InvokeLoadAlgorithm() is relevant. I think that for all cases where ignore_preload_none_ is reset before a InvokeLoadAlgorithm() call, it should actually only be set back to false if InvokeLoadAlgorithm() will set paused_ to true.

Comment 9 by foolip@chromium.org, May 30 2017

There is a spec problem here, though, namely that ignore_preload_none_ isn't explained at all. https://www.w3.org/Bugs/Public/show_bug.cgi?id=28921 is about that for load(). Spec PRs welcome :)
This is where https://codereview.chromium.org/1522463003 this refactoring was done. Before this change load algorithm and selection algorithm are split across prepareForLoad(), loadInternal() and prepareToPlay() and ignore_preload_none_(formerly known as m_havePreparedToPlay) is being handled in prepareForLoad() and prepareToPlay(). And this refactoring (https://codereview.chromium.org/1522463003) to match spec was done partially with a TODO to take care of ignore_preload_none_ in later CLs. And my attempt to implement resource selection algorithm per spec in https://codereview.chromium.org/1810513002 was suspended due to dependency on loader.
Regarding the "dependency on loader", is that only about cancelable tasks, or is it a larger problem?
It is only about cancelable tasks (the time at which microtask is fired is creating problem for media document related test cases).
This is the problematic code
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Document.cpp?q=document.cpp+package:%5Echromium$&l=5489

There is a FIXME for that piece of code (https://bugs.chromium.org/p/chromium/issues/detail?id=425790) but that doesn't look like being fixed anytime soon.
Thanks for the feedback. Do let me know if I should continue working on the issue and I can fix the issue further.

Thanks,
George.

If this is affecting real sites, then fixing it with a workaround would be reasonable. Perhaps it would work to change HTMLMediaElement::LoadResource so that we never defer the load if not paused?
Hello foolip and  mlamouri, 

 I have updated the patch and added a test. All the tests should now pass. Please do review and let me know your thoughts.

Thanks.
Hello foolip and mlamouri, 
 
 Would you be able to review the patch anytime : https://codereview.chromium.org/2910683003/

Thanks,
George.
Hello Mounir, 

 I have uploaded the new set of changes here:
https://chromium-review.googlesource.com/c/chromium/src/+/867920

Thanks,
George.

Sign in to add a comment