Closed Bug 1514016 Opened 11 months ago Closed 10 months ago

[ MediaRecorder ] New Pause/resume events in 65 fire synchronously, which is web incompatible.

Categories

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

65 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla66
Tracking Status
firefox-esr60 --- unaffected
firefox64 --- unaffected
firefox65 + fixed
firefox66 + fixed

People

(Reporter: jib, Assigned: pehrsons)

References

Details

Attachments

(11 files)

47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
Bug 1458538 added firing of pause and resume events, but they're fired synchronously, which is generally problematic (JS reentrant), not to spec, and not what Chrome does.

The spec has an issue [1] which is likely to resolve it in favor of Chrome for web compatibility, so we should work like Chrome.

STRs:
  Open https://jsfiddle.net/jib1/dzntc0bu/

Expected result (Chrome):
  Draw Blue!
  paused
  Drew Blue!
  microtask
  onpause: paused
  Draw Green!
  recording
  Drew Green!
  microtask
  onresume: recording

Actual result:
  Draw Blue!
  onpause: paused
  paused
  Drew Blue!
  microtask
  Draw Green!
  onresume: recording
  recording
  Drew Green!
  microtask

Basically, "onpause:" fires before pause() returns, and "onresume" fires before resume() returns.

[1] Like Firefox, Chrome differs from the spec in that state changes are synchronously observable, but Chrome follows the spec WRT firing events (queue a task). In https://github.com/w3c/mediacapture-record/issues/123#issuecomment-324881548 we're leaning toward resolving the spec issue by updating the spec to match Chrome.
Assignee: nobody → bvandyk
We should also update the relevant web-platform tests to validate the event timing.
> Bug 1458538 added firing of pause and resume events, but they're fired synchronously

Er, what?  This was in my review feedback on bug 1458538.  The review granted there was conditional on actually addressing the feedback, and the feedback was not addressed.  Why did that land in that state, exactly?  How do we keep this from happening again?

Anyway, the right thing here is to actually implement the spec.  If the spec is going to change too, we can track that change, but that's just going to affect when the state change happens, not when the events are fired.
Flags: needinfo?(k.bumsik)
Flags: needinfo?(jib)
> We should also update the relevant web-platform tests to validate the event timing.

This was _also_ one of my review comments on bug 1458538.
Flags: needinfo?(jib) → needinfo?(jyavenard)
[Tracking Requested - why for this release]: Not too late to fix these events before 65 hits release.

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #2)
> Anyway, the right thing here is to actually implement the spec.  If the spec
> is going to change too, we can track that change, but that's just going to
> affect when the state change happens, not when the events are fired.

(as discussed on irc) I don't think we should change when the state changes at this point, to avoid webcompat breakage.

I'll write a PR for https://github.com/w3c/mediacapture-record/issues/123 to fix the spec (change state sync but fire the event async).
Driveby change to bring our logging here in line with out other logging.

Depends on D14488
Proposed code changes up for review. Will add wpt tests once we have spec changes ironed out. Will not land until then, but figure we can make sure the code is good now.
I'll reiterate here what I said on D14488.

> I think we should also act on https://github.com/w3c/mediacapture-record/issues/150 and go the chrome way both in spec and impl, as that makes most sense.

jib, would that be a simple PR to write on top of PR #157?
Flags: needinfo?(jib)
Hi, I'm the contributor of bug 1458538. It was my mistake that I thought Chrome fires the events synchronously as well so that I left it  that way. As it was my first time contributions to Firefox I was not sure how to resolve the feedback exactly.
Flags: needinfo?(k.bumsik)
Bumsik Kim, don't worry about it.  Jean-Yves and I should have caught it between the two of us...
Flags: needinfo?(jyavenard)
During review of bug 1458538, :bz noted that our event dispatching code could be
simplified by using DOMEventTargetHelper::DispatchTrustedEvent. As this was not
done during that bug, driveby fix here while we're making other changes.
Tracking to make sure we follow up for 65.
(In reply to Bryce Seager van Dyk (:bryce) from comment #9)
> Proposed code changes up for review. Will add wpt tests once we have spec
> changes ironed out. Will not land until then, but figure we can make sure
> the code is good now.

https://github.com/w3c/mediacapture-record/pull/157 has been merged, so you can go ahead!

(In reply to Andreas Pehrson [:pehrsons] from comment #12)
> jib, would that be a simple PR to write on top of PR #157?

Yes, I've submitted https://github.com/w3c/mediacapture-record/pull/158.
Flags: needinfo?(jib)
I have some tests locally (inlcuding some tweaks to the wpt MediaRecorder-stop tests after talking to :jib), but verifying them has turned into me chasing issues in ./mach wpt. It's unclear to me how long it's going to take to get the harness to cooperate, and as I'm trying to clear some other issues before the holidays come around, this may need to go on the back burner for a bit.
Can you push the latest tests and patches you have somewhere? I can probably take a look tomorrow.
Flags: needinfo?(bvandyk)
Tests up. Notes:
- I believe in Firefox the test recorder is not firing the start event so they time out. IIRC we're not firing the start event until our encoders are initialized -- and because we're not updating the canvas our pipeline doesn't get more data and we don't init. Imagine we can work around this by drawing to the canvas and causing some more data to be pushed through the pipeline.
- Based on discussions with :jib, attempted reworked of the stop test while we're in here.
- Both tests are WIP, and I wasn't able to get the harness to run them in Chrome, so am not certain what the results are there.
Flags: needinfo?(bvandyk)
(In reply to Bryce Seager van Dyk (:bryce) from comment #23)
> Tests up. Notes:
> - I believe in Firefox the test recorder is not firing the start event so
> they time out. IIRC we're not firing the start event until our encoders are
> initialized -- and because we're not updating the canvas our pipeline
> doesn't get more data and we don't init. Imagine we can work around this by
> drawing to the canvas and causing some more data to be pushed through the
> pipeline.

Right, I discussed this with jib yesterday. Our behaviour is following an old spec. Nowadays start() should queue a task that fires "start" immediately. To not have that block this, perhaps we can land this with expected-fail.


> - Based on discussions with :jib, attempted reworked of the stop test while
> we're in here.
> - Both tests are WIP, and I wasn't able to get the harness to run them in
> Chrome, so am not certain what the results are there.

Gotcha.


Let's also land the idempotent pause() and resume() here (with wpt). Do you have a patch for the c++ work of that, or should I put one up?
Flags: needinfo?(bvandyk)
Landing with expected fail sounds sensible.

I don't have any patches for the idempotence changes, so if you're able to put them up that would be appreciated.
Flags: needinfo?(bvandyk)
I'll look at this now, to try and prevent shipping the non-compliant code in 65 to just change it again in 66.
Blocks: 1518105

Bryce, can you add D14489 as parent revision to D14571 (to have a complete stack), and abandon D14891 and D14892 (I'll re-push them with some fixes)? Thanks.

Assignee: bvandyk → apehrson
Blocks: 1458538
Status: NEW → ASSIGNED
No longer depends on: 1458538
Flags: needinfo?(bvandyk)
Attachment #9032242 - Attachment description: Bug 1514016 - WIP: Add wpt for MediaRecorder pasue and resume behaviour. → Bug 1514016 - Add WPT for MediaRecorder pause and resume behaviour. r?jib
Attachment #9032243 - Attachment description: Bug 1514016 - WIP: Rework MediaRecorder-stop tests to use async await. → Bug 1514016 - Rework MediaRecorder-stop tests to use async-await. r?jib

Scratch the abandoning bit. I was able to update your commits.

Parent set up on review board. Let me know should D14488 (currently marked as need revisions) need further work on my behalf.

Flags: needinfo?(bvandyk)

We've only got one 65 Beta left this cycle before the RC. I'm thinking we should probably just focus on shipping this in 66 at this point. What are your thoughts, Jan-Ivar?

Flags: needinfo?(jib)

Going to try landing this after discussions with :jib.

Pushed by bvandyk@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7a0378016cf9
Add DispatchEventRunnable to media recorder to aid with dispatching different named events. r=pehrsons
https://hg.mozilla.org/integration/autoland/rev/e19d8db0f4d1
Replace DispatchStartEventRunnable with DispatchEventRunnable. r=pehrsons
https://hg.mozilla.org/integration/autoland/rev/8f1376f47212
Fire pause and resume events asynchronously. r=pehrsons
https://hg.mozilla.org/integration/autoland/rev/7f454762898b
Add logging of this to MediaRecorder::pause and resume. r=pehrsons
https://hg.mozilla.org/integration/autoland/rev/1b35c6f6a582
Use DispatchTrustedEvent helper in MediaRecorder::DispatchSimpleEvent. r=pehrsons
https://hg.mozilla.org/integration/autoland/rev/e98e7b77182b
Make MediaRecorder::Stop idempotent. r=jib
https://hg.mozilla.org/integration/autoland/rev/bdfb537d6564
Make MediaRecorder::Pause and MediaRecorder::Resume idempotent. r=jib
https://hg.mozilla.org/integration/autoland/rev/3556aa3e0f58
Add WPT for MediaRecorder pause and resume behaviour. r=jib
https://hg.mozilla.org/integration/autoland/rev/8183cba74edd
Rework MediaRecorder-stop tests to use async-await. r=jib
https://hg.mozilla.org/integration/autoland/rev/b7b6a2978011
Check that MediaRecorder::Stop is idempotent. r=jib
https://hg.mozilla.org/integration/autoland/rev/3b55c65ded33
Check that all events are fired as expected for MediaRecorder-stop.html. r=jib

(In reply to Ryan VanderMeulen [:RyanVM] from comment #35)

We've only got one 65 Beta left this cycle before the RC. I'm thinking we should probably just focus on shipping this in 66 at this point. What are your thoughts, Jan-Ivar?

It's all green and ready to go from last week, except landing it fell through the cracks somehow. Landing now.

I'd still prefer we to take it if possible — it's less than 100 lines without tests, and we think it is low risk. Otherwise, this new behavior is wrong out of the gate, which stinks a bit for web developers. We did two spec changes already for this.

Flags: needinfo?(jib)

Please nominate this for Beta approval when you get a chance.

Flags: needinfo?(apehrson)

Comment on attachment 9034957 [details]
Bug 1514016 - Check that all events are fired as expected for MediaRecorder-stop.html. r?jib

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1458538

User impact if declined: MediaRecorder's pause and resume events will begin working, except with the wrong timing which is not to spec and not how Chrome works. Events will fire too soon and synchronously (reentering JS), potentially causing web compat problems with any web site using the MediaRecorder and reacting to these events expecting Chrome/spec behavior.

Mitigating circumstances are that we don't fire these events in release today, so any site relying on them presumably already doesn't work right in release. However, not firing the events may have different side-effects than firing them too soon, so this change may cause behavioral changes by executing content JS out of order that wouldn't have run otherwise.

Also, minor: recorder.stop() and recorder.pause() won't be idempotent and will throw errors if called twice in a row, violating the latest spec, but that part is not a regression.

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: No

If yes, steps to reproduce: comment 0.

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): Low risk, as only MediaRecorder API is impacted, and less than 100 lines of code changed, the rest are tests, which are green in Nightly. Tested in Nightly using steps in comment 0.

String changes made/needed: None.

Attachment #9034957 - Flags: approval-mozilla-beta?

Request is for all patches in this bug. Andreas is on PTO, back Thursday FWIW.

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/14870 for changes under testing/web-platform/tests

Comment on attachment 9034957 [details]
Bug 1514016 - Check that all events are fired as expected for MediaRecorder-stop.html. r?jib

[Triage Comment]
While the number of patches for this is high, a lot of them are test changes. The actual code changes are pretty small. Given that this is a new feature shipping in Fx65, it makes sense to fix up the spec compliance issues before it goes out the door. Approved for 65.0b12.

Flags: needinfo?(apehrson)
Attachment #9034957 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.