Closed Bug 1363915 Opened 7 years ago Closed 5 years ago

Pause, and resume events are not emitted by the MediaRecorder, nor are handlers exposed for these events

Categories

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

defect

Tracking

()

RESOLVED DUPLICATE of bug 1458538

People

(Reporter: bryce, Assigned: bryce)

References

Details

(Keywords: dev-doc-needed)

Attachments

(3 files)

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
Taking this to clear the block on writing some tests for bug 1354457.
Assignee: nobody → bvandyk
Blocks: 1354457
We also do not appear to emit the stop event. Updated test and bug title to reflect this.
Summary: Pause and resume events are not emitted by the MediaRecorder → Pause, resume, and stop events are not emitted by the MediaRecorder
Nope, I was mistaken, looks like an old version of the example was running. Stop looks okay. Just pause and resume not working.
Summary: Pause, resume, and stop events are not emitted by the MediaRecorder → Pause, and resume events are not emitted by the MediaRecorder
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.
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.
Summary: Pause, and resume events are not emitted by the MediaRecorder → Pause, and resume events are not emitted by the MediaRecorder, nor are handlers exposed for these events
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.
Attachment #8866597 - Flags: review?(pehrson)
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.
Attachment #8866604 - Flags: review?(pehrson)
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.
Attachment #8866597 - Flags: review?(jib)
Rank: 21
Priority: -- → P2
(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/
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
Attachment #8866597 - Flags: review?(jib) → review-
Comment on attachment 8866604 [details]
Bug 1363915 - Update MediaRecorder to expose onpause and onresume event handlers.

https://reviewboard.mozilla.org/r/138212/#review141842
Attachment #8866604 - Flags: review+
(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.
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?
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?
Mass change P2->P3 to align with new Mozilla triage process.
Priority: P2 → P3
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.
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: needinfo?(bvandyk)
Resolution: --- → DUPLICATE
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
Flags: needinfo?(bvandyk)
You need to log in before you can comment on or make changes to this bug.