Closed Bug 1306999 Opened 8 years ago Closed 5 years ago

Intermittent dom/media/test/test_streams_individual_pause.html | video1 video frame should not have updated since video1 paused - got "r0g0b0a0", expected "r0g255b0a255"

Categories

(Core :: WebRTC: Audio/Video, defect, P3)

Product:
Core
Component:
WebRTC: Audio/Video
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla69
Tracking Flags:
Tracking Status
firefox69 --- fixed

People

(Reporter: intermittent-bug-filer, Assigned: pehrsons)

References

Details

(Keywords: dev-doc-needed, intermittent-failure)

Attachments

(11 files)

Bug 1306999 - Change from "playing" to "loadeddata" before assuming there's a video frame. r?jib
(deleted), text/x-phabricator-request
Details
Bug 1306999 - Revert temporary fix. r?jib
(deleted), text/x-phabricator-request
Details
Bug 1306999 - Add mochitest. r?jib
(deleted), text/x-phabricator-request
Details
Bug 1306999 - Add WPT. r?jib
(deleted), text/x-phabricator-request
Details
Bug 1306999 - Move HTMLMediaElement's VideoFrameListener to a VideoOutput. r?jib
(deleted), text/x-phabricator-request
Details
Bug 1306999 - Load the first frame of a MediaStream with video into a media element when not playing or autoplaying. r?jib
(deleted), text/x-phabricator-request
Details
Bug 1306999 - Use ProducerIDs in VideoOutput. r?padenot
(deleted), text/x-phabricator-request
Details
Bug 1306999 - Invalidate HTMLVideoElement after resetting videoWidth and videoHeight. r?jya
(deleted), text/x-phabricator-request
Details
Bug 1306999 - Reset mSrcStreamPlaybackEnded when unsetting srcObject. r?padenot
(deleted), text/x-phabricator-request
Details
Bug 1306999 - Modernize test_streams_individual_pause.html. r?jib
(deleted), text/x-phabricator-request
Details
Bug 1306999 - Make HTMLMediaElement::CanActivateAutoplay() spec compliant. r?jya!,jib!
(deleted), text/x-phabricator-request
Details
Component: Audio/Video → Audio/Video: MediaStreamGraph
We pause a video element playing a MediaStream and it's visible image becomes black. Even with no alpha. It's as if the drawImage() call bailed and nothing was drawn to the canvas.
Rank: 35
Priority: -- → P3
We actually have a screenshot here that shows both video elements displaying images at different times. That definitely points to the drawImage() call.
Rank: 35
Component: Audio/Video: MediaStreamGraph → Canvas: 2D
Priority: P3 → --
https://wiki.mozilla.org/Bugmasters#Intermittent_Test_Failure_Cleanup
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → INCOMPLETE
Status: RESOLVED → REOPENED
Resolution: INCOMPLETE → ---
https://wiki.mozilla.org/Bug_Triage#Intermittent_Test_Failure_Cleanup
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago
Resolution: --- → INCOMPLETE
Status: RESOLVED → REOPENED
Resolution: INCOMPLETE → ---
I caught this in rr. The reference frame for v1 is captured when v1 is in HAVE_NOTHING, so clearly there's nothing to show. The odd thing is that we get "playing" in HAVE_NOTHING, which is against the spec. This is due to [1] where we don't check that we have a frame to display when checking whether we are eligible for autoplay. And since this is for a MediaStream which has only live-data we won't have a frame to display until we are playing. We don't display the current frame as soon as it's been loaded, as we do for playback. Perhaps we should. I don't think the spec is clear about how to handle this, but it will require some spec language archeology to figure out. [1] https://searchfox.org/mozilla-central/rev/3d989d097fa35afe19e814c6d5bc2f2bf10867e1/dom/html/HTMLMediaElement.cpp#6330
Rank: 25
Component: Canvas: 2D → WebRTC: Audio/Video
Priority: -- → P3
I found [2]: > When the video element is paused, the current playback position is the first frame of video, and the element's show poster flag is set > The video element represents its poster frame, if any, or else the first frame of the video. This in itself sounds a bit contradictory. What if the show poster flag is not set? Then it do we do the "or else the first frame" or fall through to the next: > The video element represents the last frame of the video to have been rendered. There wouldn't be a "last frame to have been rendered" yet. I suppose we'd go with the "or else the first frame of the video". This then becomes a question on whether MediaStreams have a "first frame of the video". [2] https://html.spec.whatwg.org/multipage/media.html#the-video-element:dom-media-paused
Bug 1423241 has made this spike.
Assignee: nobody → apehrson
Blocks: 1423241
Status: REOPENED → ASSIGNED
jya, do you have thoughts on how we should behave here? In particular this bit: (In reply to Andreas Pehrson [:pehrsons] from comment #20) > I suppose we'd go with the "or else the first frame of the video". This then > becomes a question on whether MediaStreams have a "first frame of the video". Since MediaStreams are not buffered, a "first frame of the video" would be outdated immediately after it's been displayed. How does this work for streaming through the playback stack? Such streams would be buffered, right?
Flags: needinfo?(jyavenard)
This patch is a stop-gap fix for the intermittent. I plan to follow up with a proper fix after this lands, if we can reach come to a conclusion on how it should be.
Keywords: leave-open
Pushed by pehrsons@gmail.com: https://hg.mozilla.org/integration/autoland/rev/759ec08eebaa Change from "playing" to "loadeddata" before assuming there's a video frame. r=jib
Flags: needinfo?(jyavenard)
I'd still like your thoughts on comment 24 so we can proceed with a long-term solution to this.
Flags: needinfo?(jyavenard)
playing is fired when you call play(), irrespective of the readyState. (In reply to Andreas Pehrson [:pehrsons] from comment #24) > jya, do you have thoughts on how we should behave here? > > In particular this bit: > > (In reply to Andreas Pehrson [:pehrsons] from comment #20) > > I suppose we'd go with the "or else the first frame of the video". This then > > becomes a question on whether MediaStreams have a "first frame of the video". > > Since MediaStreams are not buffered, a "first frame of the video" would be > outdated immediately after it's been displayed. > > > How does this work for streaming through the playback stack? Such streams > would be buffered, right? A frame is displayed until there's a new one ready to be displayed. Regardless of buffering. I'm not sure I understand the problem at hand however. "loadeddata" is fired when we have a frame, as such, if the element is playing you can't guarantee that what you see will be the first frame, it may be the next one and so forth. loadeddata is *queued* when there's a first frame, playback is asynchronous so by the time loadeddata actually is fired it's likely it would no longer be the first frame. With playback what you would do here is not call play at all. when loadeddata is fired you're sure to look at the first frame. Waiting on playing was wrong regardless.
Flags: needinfo?(jyavenard)
(In reply to Jean-Yves Avenard [:jya] from comment #31) > playing is fired when you call play(), irrespective of the readyState. This test doesn't play(). It's using autoplay. So "playing" should fire when we enter HAVE_ENOUGH_DATA. > (In reply to Andreas Pehrson [:pehrsons] from comment #24) > > jya, do you have thoughts on how we should behave here? > > > > In particular this bit: > > > > (In reply to Andreas Pehrson [:pehrsons] from comment #20) > > > I suppose we'd go with the "or else the first frame of the video". This then > > > becomes a question on whether MediaStreams have a "first frame of the video". > > > > Since MediaStreams are not buffered, a "first frame of the video" would be > > outdated immediately after it's been displayed. > > > > > > How does this work for streaming through the playback stack? Such streams > > would be buffered, right? > > A frame is displayed until there's a new one ready to be displayed. > Regardless of buffering. > > I'm not sure I understand the problem at hand however. > > "loadeddata" is fired when we have a frame, as such, if the element is > playing you can't guarantee that what you see will be the first frame, it > may be the next one and so forth. > > loadeddata is *queued* when there's a first frame, playback is asynchronous > so by the time loadeddata actually is fired it's likely it would no longer > be the first frame. > > With playback what you would do here is not call play at all. when > loadeddata is fired you're sure to look at the first frame. > > Waiting on playing was wrong regardless. Well no, I interpret it as correct since we're relying on autoplay to fire "playing". For autoplay, the first frame should be visible while paused, then we start playing automatically when entering HAVE_ENOUGH_DATA and video frames continue from the first frame. But for a MediaStream we don't behave in this way. We don't check for a frame so we start playing in HAVE_NOTHING. To me it then boils down to whether a MediaStream is considered having a first frame or not. If it does, well that's weird since we don't buffer the frames and there'll be an observable jump once playing starts. If it doesn't, well then I have followup questions on when playing should be fired. I'm not sure which makes more sense and that's the dilemma here atm.
Flags: needinfo?(jyavenard)
I filed https://github.com/whatwg/html/issues/4215 to try to progress this issue.

The spec issue seems clear -- we should display the first frame even when paused. This will lead to the jump once playing starts, but that's all right. We should be able to align the MediaStream handling in HTMLMediaElement more to that of playback with this change.

Adding dev-doc-needed because this will be a very observable change.

Flags: needinfo?(jyavenard)
Keywords: leave-open → dev-doc-needed
Attached file Bug 1306999 - Add mochitest. r?jib (deleted) —

Depends on D33289

Attached file Bug 1306999 - Add WPT. r?jib (deleted) —

Depends on D33290

This allows it to intercept frames in the rendering pipe, so that we don't have
to duplicate the logic for converting VideoChunks to NonOwningImages.

Depends on D33291

With the extra first-frame-logic in HTMLMediaElement there are two VideoOutput
instances feeding frames to the same ImageContainer. To maintain FrameID
invariants these need to have unique ProducerIDs.

Depends on D33293

Unsetting a playing video element's MediaStream-srcObject attribute will
otherwise leave the element displaying the latest frame of the video track.

Depends on D33294

Depends on D33296

Attachment #9068991 - Attachment description: Bug 1306999 - Move HTMLMediaElement's VideoFrameListener to a VideoOutput. r?padenot → Bug 1306999 - Move HTMLMediaElement's VideoFrameListener to a VideoOutput. r?jib
Attachment #9068992 - Attachment description: Bug 1306999 - Load the first frame of a MediaStream with video into a media element when not playing or autoplaying. r?padenot → Bug 1306999 - Load the first frame of a MediaStream with video into a media element when not playing or autoplaying. r?jib

Note that what needs a dev-doc here is that loading a MediaStream in a HTMLMediaElement is now spec-compliant.

The change that makes it so is that we now load a video frame automatically after assigning a MediaStream with a video MediaStreamTrack to the srcObject attribute. Previously we wouldn't load a frame until the element was playing.

Js-observable changes here are most notably that there are a bunch of new events fired on a HTMLMediaElement that does not have the autoplay attribute set to true, and whose play() method isn't called; including "loadedmetadata", "loadeddata", "canplay" and "canplaythrough". This means that the readyState attribute will also automatically advance to HAVE_ENOUGH_DATA once a frame is available.

User-visible changes are most notably that the media element will load the first video frame of the selected video track (NB in case of multiple video tracks, selected here is just internal) and display it as a still frame until the application calls play() or sets the autoplay attribute to true.

Pushed by pehrsons@gmail.com: https://hg.mozilla.org/integration/autoland/rev/5b2a3e5bd246 Revert temporary fix. r=jib https://hg.mozilla.org/integration/autoland/rev/bd60d9099b47 Add mochitest. r=jib https://hg.mozilla.org/integration/autoland/rev/32c521c1b373 Add WPT. r=jib https://hg.mozilla.org/integration/autoland/rev/c1473d84bdec Move HTMLMediaElement's VideoFrameListener to a VideoOutput. r=jib https://hg.mozilla.org/integration/autoland/rev/e13f35cff432 Load the first frame of a MediaStream with video into a media element when not playing or autoplaying. r=jib https://hg.mozilla.org/integration/autoland/rev/143bc31d3d70 Use ProducerIDs in VideoOutput. r=padenot https://hg.mozilla.org/integration/autoland/rev/d83d0d34de2a Invalidate HTMLVideoElement after resetting videoWidth and videoHeight. r=jya https://hg.mozilla.org/integration/autoland/rev/f03b1a7c02ab Reset mSrcStreamPlaybackEnded when unsetting srcObject. r=padenot https://hg.mozilla.org/integration/autoland/rev/375a2a438161 Modernize test_streams_individual_pause.html. r=jib https://hg.mozilla.org/integration/autoland/rev/0ff338ed7e92 Make HTMLMediaElement::CanActivateAutoplay() spec compliant. r=jya,jib
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/17254 for changes under testing/web-platform/tests

(In reply to Web Platform Test Sync Bot from comment #51)

Can't merge web-platform-tests PR due to failing upstream checks:
Github PR https://github.com/web-platform-tests/wpt/pull/17254

Why would it try to verify the added WPT tests on current Nightly when there are also pending platform changes...?

Regressions: 1558646

Why would it try to verify the added WPT tests on current Nightly when there are also pending platform changes...?

Because the system isn't nearly clever enough to know when test changes are accompanied by platform changes that mean the stability checks ought to be deferred until some future revision is available.

I can force-merge the PR.

(In reply to James Graham [:jgraham] from comment #54)

Why would it try to verify the added WPT tests on current Nightly when there are also pending platform changes...?

Because the system isn't nearly clever enough to know when test changes are accompanied by platform changes that mean the stability checks ought to be deferred until some future revision is available.

I can force-merge the PR.

Ok, thanks!

What revision does it use for the tests? In case I were to try to work around this by postponing the landing of just the WPT changes.
And is there a tracking bug for making it clever enough to figure this out? It feels like it will be quite frequent, at least in my regular workflow.

Flags: needinfo?(james)

The tests are running on GitHub and it doesn't know anything about the Gecko release process; it just pulls the latest nightly to run the tests. It's quite hard to even think about how it would work otherwise; finding some way to use the build from our CI just for upstreamed changes might be possible? But it would certainly not be a small amount of work to handle synchonising the two different systems. I think in the foreseeable future this is likely to be something that's handled manually. FWIW when tests from Gecko only fail stability checks in Firefox I tend to assume that landing them upstream isn't making things worse than they are in m-c and so end up force merging.

Flags: needinfo?(james)

(In reply to James Graham [:jgraham] from comment #57)

The tests are running on GitHub and it doesn't know anything about the Gecko release process; it just pulls the latest nightly to run the tests. It's quite hard to even think about how it would work otherwise; finding some way to use the build from our CI just for upstreamed changes might be possible? But it would certainly not be a small amount of work to handle synchonising the two different systems. I think in the foreseeable future this is likely to be something that's handled manually. FWIW when tests from Gecko only fail stability checks in Firefox I tend to assume that landing them upstream isn't making things worse than they are in m-c and so end up force merging.

Without knowing any details..,

The initial comment on github PR has the gecko rev and repo [1], and from those you can figure out what to query hg.mo with to get to the try run on treeherder [2]. Can those be parsed out, and then have it wait for the appropriate taskcluster build to complete, before pulling and running on that?

Or wait for the bugzilla bug to become RESOLVED, and another Nightly build released?

[1] https://github.com/web-platform-tests/wpt/pull/17254
[2] https://hg.mozilla.org/integration/autoland/rev/32c521c1b373336081db70bad913793a1421568c

The upstream PR has gotten "merged" a bit too many times now. James, is everything in order?

Flags: needinfo?(james)

I don't think anything bad is happening apart from the spam, but yeah it shouldn't be doing that. Filed https://github.com/mozilla/wpt-sync/issues/377

Flags: needinfo?(james)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: