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

As per the MediaRecorder spec, we should fire a pause event when pausing[1] and a resume event when resuming[2]. We do not appear to be doing so. Aside from spec compliance and third party usage, this is useful for us to enable automated testing.

Steps to reproduce:
1. Visit https://jsfiddle.net/SingingTree/psctg12c/
2. Begin recording.
3. While recording attempt to pause and resume the recording
4. No output is produced on the test page indicating a pause or resume

Expected Behaviour:
During 4. I expect to see a log for pause and resume events based on the registered event handlers.

[1]: https://w3c.github.io/mediacapture-record/MediaRecorder.html#dom-mediarecorder-pause
[2]: https://w3c.github.io/mediacapture-record/MediaRecorder.html#dom-mediarecorder-resume

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

Taking this to clear the block on writing some tests for bug 1354457.

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

We also do not appear to emit the stop event. Updated test and bug title to reflect this.

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

Nope, I was mistaken, looks like an old version of the example was running. Stop looks okay. Just pause and resume not working.

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

Note, it appears that the 'onpause' and 'onresume' handlers for MediaRecorder are also not implemented. However, these are not blocking issues. I intend to resolve this also, but will do so in a follow up bug, to keep my current yak shaving to a minimum.

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

Seems like a good day for immediately contradicting my previous comments. It would further simplify the tests to expose the handlers mentioned in #5 and upon investigation isn't looking like too much more work. So having a go as part of this bug.

	Andreas Pehrson [:pehrsons]
	Comment 8 • 7 years ago
mozreview-review

Comment on attachment 8866597 [details]
Bug 1363915 - Update MediaRecorder to fire events when pausing and resuming.

https://reviewboard.mozilla.org/r/138202/#review141498

::: dom/media/MediaRecorder.cpp:537
(Diff revision 1)
>      NS_ENSURE_TRUE(mTrackUnionStream, NS_ERROR_FAILURE);
>      mTrackUnionStream->Suspend();
>      if (mEncoder) {
>        mEncoder->Suspend();
>      }
> +    mRecorder->DispatchSimpleEvent(NS_LITERAL_STRING("pause"));

Sync-dispatching "pause" in pause() is a little odd to me.

In mediacapture-main this pattern was undesired (MediaStreamTrack.stop(), etc.) because the application knows what it's doing and is fully capable of calling its own handler.

Same applies to "resume".

What's the implementation status of this in other browsers?

I'll cancel my review. I think jib would be a better candidate to review this.

	Andreas Pehrson [:pehrsons]
	Comment 9 • 7 years ago
mozreview-review

Comment on attachment 8866604 [details]
Bug 1363915 - Update MediaRecorder to expose onpause and onresume event handlers.

https://reviewboard.mozilla.org/r/138212/#review141502

This requires review from a dom peer.

	Andreas Pehrson [:pehrsons]
	Comment 10 • 7 years ago

Comment on attachment 8866597 [details]
Bug 1363915 - Update MediaRecorder to fire events when pausing and resuming.

jib, thoughts on these events? I find them questionable, see comment 8.

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

(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #8)
> 
> What's the implementation status of this in other browsers?
> 

Chrome and Opera (MDN doesn't reflect Opera correctly) both implement both the firing of these events and the handlers. Here's a fiddle where the handlers are explicitly logged for anyone interested in checking: https://jsfiddle.net/SingingTree/psctg12c/13/

	Jan-Ivar Bruaroey [:jib] (needinfo? me)
	Comment 12 • 7 years ago
mozreview-review

Comment on attachment 8866597 [details]
Bug 1363915 - Update MediaRecorder to fire events when pausing and resuming.

https://reviewboard.mozilla.org/r/138202/#review141790

::: dom/media/MediaRecorder.cpp:537
(Diff revision 1)
>      NS_ENSURE_TRUE(mTrackUnionStream, NS_ERROR_FAILURE);
>      mTrackUnionStream->Suspend();
>      if (mEncoder) {
>        mEncoder->Suspend();
>      }
> +    mRecorder->DispatchSimpleEvent(NS_LITERAL_STRING("pause"));

I agree with pehrsons that sync-dispatching is wrong. It causes pause event handlers to run before pause() has even returned. Rather we should queue a task like the spec says, and like Chrome does.

Here's a modified fiddle that highlights timing differences in Chrome vs. Firefox: https://jsfiddle.net/jib1/psctg12c/17/

However, the spec [1] also says we should queue a task before changing the state in both pause(), resume() and stop(). However, Chrome doesn't do this, and neither do we in stop().

We should either implement the spec, or push to change it.

[1] https://w3c.github.io/mediacapture-record/MediaRecorder.html#dom-mediarecorder-pause

	Jan-Ivar Bruaroey [:jib] (needinfo? me)
	Comment 13 • 7 years ago
mozreview-review

Comment on attachment 8866604 [details]
Bug 1363915 - Update MediaRecorder to expose onpause and onresume event handlers.

https://reviewboard.mozilla.org/r/138212/#review141842

	Jan-Ivar Bruaroey [:jib] (needinfo? me)
	Comment 14 • 7 years ago

(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #10)
> Comment on attachment 8866597 [details]
> Bug 1363915 - Update MediaRecorder to fire events when pausing and resuming.
> 
> jib, thoughts on these events? I find them questionable, see comment 8.

As a control surface to an asynchronous process, MediaRecorder uses a slightly different model than say gUM, PeerConnection, or MediaStreamTrack (*).

For one, none of the methods Start(), Stop(), Pause() or Resume() return promises. Instead, corresponding events fire when these state-changing operations finish. Stop() already works this way, so that seems good to me.

I'm not familiar with our implementation, and a bit surprised Pause() and Resume() didn't need to be asynchronous. We can always consider proposing a spec change, if we think that'll be common, though the fact that Chrome always supports this, suggests maybe we should to.

---
*) MediaStreamTrack is a bit of a mix, using both promises in the case of one method (applyConstraints()), but otherwise relies on hooking up to a video element for observing most other state.

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

Understood. I had based this on the code for stop() assuming that is was conformant. However, after reading the above I see why this is problematic. I'm going to rework this patch, any reservations about my doing so? Further down the road I expect to do some promisificaiton work in this area also.

That said, if there are reservations: are we concerned about the spec? Do we wish to see changes in how these things are handled?

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

Pushed changes that bring the event firing in line with other events. However, I haven't made changes regarding the spec conformance question, since I'd like more discussion.

On rereading the spec, is it not saying that the queued task should perform the state change itself. I.e. if we're using runnables, it's the runnable that we queue who should change the state, and fire an event, rather than the initial call?

	Jan-Ivar Bruaroey [:jib] (needinfo? me)
	Comment 20 • 7 years ago

Filed https://github.com/w3c/mediacapture-record/issues/123 seeking clarification on this.

	Bulk Bug Changes for mreavy's org
	Comment 21 • 7 years ago

Mass change P2->P3 to align with new Mozilla triage process.

	Andreas Pehrson [:pehrsons]
	Comment 22 • 5 years ago

Bryce, this was fixed under your feet in bug 1458538. Could you make a quick check of the code that landed and if there are issues related to the spec still (like comment 20 suggests)? I reckon we might need a followup for that spec resolution.

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

The changes appear to have us do the following for pause and resume:
- Sync change state
- Sync fire event

in that order.

This means we're doing what comment #8 and onward expressed concerns about. I also think it's not in line with the spec as currently written, in that both should be done in a queued task. Chrome will set state sync, but will fire the event async.

jib's fiddle in comment #12 can be used to highlight the different behaviours between us and Chrome. I have a codepen to do so too[0].

IMO we should have the state change be sync (update the spec) and the events be async (change our code).

[0]: https://codepen.io/SingingTree/pen/xmZEmO?editors=1011