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

Status: RESOLVED FIXED

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

Description

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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.

Comment 1

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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

Comment 2

[Boris Zbarsky [:bzbarsky]]

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

Comment 3

[Boris Zbarsky [:bzbarsky]]

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

Comment 4

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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

Comment 5

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Attachment: Bug 1514016 - Add DispatchEventRunnable to media recorder to aid with dispatching different named events. r?pehrsons

Comment 6

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Attachment: Bug 1514016 - Replace DispatchStartEventRunnable with DispatchEventRunnable. r?pehrsons

Depends on D14486

Comment 7

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Attachment: Bug 1514016 - Fire pause and resume events asynchronously. r?pehrsons

Depends on D14487

Comment 8

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Attachment: Bug 1514016 - Add logging of this to MediaRecorder::pause and resume. r?pehrsons

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

Depends on D14488

Comment 9

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

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.

Comment 10

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

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

Comment 11

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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

Comment 12

[Andreas Pehrson [:pehrsons] (PTO; back in Jan)]

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?

Comment 13

[Bumsik Kim]

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.

Comment 14

[Boris Zbarsky [:bzbarsky]]

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

Comment 15

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Attachment: Bug 1514016 - Use DispatchTrustedEvent helper in MediaRecorder::DispatchSimpleEvent. r?pehrsons

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.

Comment 16

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking to make sure we follow up for 65.

Comment 17

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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

Comment 18

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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

Comment 19

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

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.

Comment 20

[Andreas Pehrson [:pehrsons] (PTO; back in Jan)]

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

Comment 21

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Attachment: Bug 1514016 - Add WPT for MediaRecorder pause and resume behaviour. r?jib

Comment 22

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Attachment: Bug 1514016 - Rework MediaRecorder-stop tests to use async-await. r?jib

Depends on D14891

Comment 23

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

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.

Comment 24

[Andreas Pehrson [:pehrsons] (PTO; back in Jan)]

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

Comment 25

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

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.

Comment 26

[Andreas Pehrson [:pehrsons] (PTO; back in Jan)]

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

Comment 27

[Andreas Pehrson [:pehrsons] (PTO; back in Jan)]

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.

Comment 28

[Andreas Pehrson [:pehrsons] (PTO; back in Jan)]

Attachment: Bug 1514016 - Make MediaRecorder::Stop idempotent. r?jib

Depends on D14571

Comment 29

[Andreas Pehrson [:pehrsons] (PTO; back in Jan)]

Attachment: Bug 1514016 - Make MediaRecorder::Pause and MediaRecorder::Resume idempotent. r?jib

Comment 30

[Andreas Pehrson [:pehrsons] (PTO; back in Jan)]

Attachment: Bug 1514016 - Check that MediaRecorder::Stop is idempotent. r?jib

Comment 31

[Andreas Pehrson [:pehrsons] (PTO; back in Jan)]

Attachment: Bug 1514016 - Check that all events are fired as expected for MediaRecorder-stop.html. r?jib

Comment 32

[Andreas Pehrson [:pehrsons] (PTO; back in Jan)]

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

Comment 33

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

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

Comment 34

[Andreas Pehrson [:pehrsons] (PTO; back in Jan)]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cceb64aef6e452730ab9abbfadab9c10d92983c2

Comment 35

[Ryan VanderMeulen [:RyanVM]]

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?

Comment 36

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Going to try landing this after discussions with :jib.

Comment 37

[Pulsebot]

Pushed by [e-mail]:
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

Comment 38

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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

Comment 39

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/7a0378016cf9
https://hg.mozilla.org/mozilla-central/rev/e19d8db0f4d1
https://hg.mozilla.org/mozilla-central/rev/8f1376f47212
https://hg.mozilla.org/mozilla-central/rev/7f454762898b
https://hg.mozilla.org/mozilla-central/rev/1b35c6f6a582
https://hg.mozilla.org/mozilla-central/rev/e98e7b77182b
https://hg.mozilla.org/mozilla-central/rev/bdfb537d6564
https://hg.mozilla.org/mozilla-central/rev/3556aa3e0f58
https://hg.mozilla.org/mozilla-central/rev/8183cba74edd
https://hg.mozilla.org/mozilla-central/rev/b7b6a2978011
https://hg.mozilla.org/mozilla-central/rev/3b55c65ded33

Comment 40

[Ryan VanderMeulen [:RyanVM]]

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

Comment 41

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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.

Comment 42

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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

Comment 43

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

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

Comment 44

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 45

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/7746b834faec
https://hg.mozilla.org/releases/mozilla-beta/rev/eb356ac31743
https://hg.mozilla.org/releases/mozilla-beta/rev/90eb6008a5ae
https://hg.mozilla.org/releases/mozilla-beta/rev/5d242f707c45
https://hg.mozilla.org/releases/mozilla-beta/rev/9e2fe8635c95
https://hg.mozilla.org/releases/mozilla-beta/rev/e74775e3a741
https://hg.mozilla.org/releases/mozilla-beta/rev/f8fa8bc2b93f
https://hg.mozilla.org/releases/mozilla-beta/rev/662ea26b9f34
https://hg.mozilla.org/releases/mozilla-beta/rev/f11dd983a8aa
https://hg.mozilla.org/releases/mozilla-beta/rev/c8acac1e463c
https://hg.mozilla.org/releases/mozilla-beta/rev/944e9c24989d