Jan-Ivar Bruaroey [:jib] (needinfo? me) Reporter
	Description • 5 years ago

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.

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Reporter
	Comment 1 • 5 years ago

We should also update the relevant web-platform tests to validate the event timing.

	Boris Zbarsky [:bzbarsky]
	Comment 2 • 5 years ago

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

	Boris Zbarsky [:bzbarsky]
	Comment 3 • 5 years ago

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

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Reporter
	Comment 4 • 5 years ago • Edited

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

	Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail
	Comment 6 • 5 years ago

Depends on D14486

	Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail
	Comment 7 • 5 years ago

Depends on D14487

	Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail
	Comment 8 • 5 years ago

Driveby change to bring our logging here in line with out other logging.

Depends on D14488

	Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail
	Comment 9 • 5 years ago

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.

	Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail
	Comment 10 • 5 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=76f49de1093e2c9e20416fca0eefde366fa83183

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Reporter
	Comment 11 • 5 years ago

Spec PR is up https://github.com/w3c/mediacapture-record/pull/157

	Andreas Pehrson [:pehrsons] Assignee
	Comment 12 • 5 years ago

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?

	Bumsik Kim
	Comment 13 • 5 years ago

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.

	Boris Zbarsky [:bzbarsky]
	Comment 14 • 5 years ago

Bumsik Kim, don't worry about it.  Jean-Yves and I should have caught it between the two of us...

	Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail
	Comment 15 • 5 years ago

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.

	Liz Henry (:lizzard) (relman/hg->git project)
	Comment 16 • 5 years ago

Tracking to make sure we follow up for 65.

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Reporter
	Comment 17 • 5 years ago

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

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Reporter
	Comment 18 • 5 years ago

https://github.com/w3c/mediacapture-record/pull/158 has been merged as well.

	Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail
	Comment 19 • 5 years ago

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.

	Andreas Pehrson [:pehrsons] Assignee
	Comment 20 • 5 years ago

Can you push the latest tests and patches you have somewhere? I can probably take a look tomorrow.

	Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail
	Comment 22 • 5 years ago

Depends on D14891

	Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail
	Comment 23 • 5 years ago

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.

	Andreas Pehrson [:pehrsons] Assignee
	Comment 24 • 5 years ago

(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?

	Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail
	Comment 25 • 5 years ago

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.

	Andreas Pehrson [:pehrsons] Assignee
	Comment 26 • 5 years ago

I'll look at this now, to try and prevent shipping the non-compliant code in 65 to just change it again in 66.

	Andreas Pehrson [:pehrsons] Assignee
	Comment 28 • 5 years ago

Depends on D14571

	Pulsebot
	Comment 37 • 5 years ago

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

	Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)
	Comment 43 • 5 years ago

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