Closed Bug 1354457 Opened 7 years ago Closed 7 years ago

MediaRecorder.pause() stopped working in 51

Categories

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

51 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox-esr52 --- wontfix
firefox53 --- wontfix
firefox54 --- wontfix
firefox55 --- fixed

People

(Reporter: bobyper, Assigned: bryce)

References

(Depends on 1 open bug)

Details

(Keywords: regression)

Attachments

(4 files)

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:52.0) Gecko/20100101 Firefox/52.0
Build ID: 20170323105023

Steps to reproduce:

MediaRecorder.pause() not working. In Firefox 50 worked fine. But both, Firefox 51 and 52 have this isssue


Actual results:

After calling MediaRecorder.pause() audio is paused but video continued recording


Expected results:

need audio and video pause recording. After MediaRecorder.resume() continue recording again
http://jsfiddle.net/jib1/d0sn64f8/

15:39.08 INFO: Oh noes, no (more) inbound revisions :(
15:39.08 INFO: Last good revision: 975ba208687a97ecb9fd439c1ee52bfa3350e25b
15:39.08 INFO: First bad revision: d320ef56876f52db9bc0eb79554c7332d4793769
15:39.08 INFO: Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=975ba208687a97ecb9fd439c1ee52bfa3350e25b&tochange=d320ef56876f52db9bc0eb79554c7332d4793769

Perhaps bug 1201363?
Rank: 15
Keywords: regression
Priority: -- → P1
Summary: MediaRecorder.pause() not working → MediaRecorder.pause() stopped working in 51
Version: 52 Branch → 51 Branch
Status: UNCONFIRMED → NEW
Ever confirmed: true
I knew this was somewhat broken with direct listeners pushing data ahead, but not that it was completely broken.

Bryce, do you want to take this and see if we can employ a quick fix and a test? Bug 1296531 will otherwise fix this when it lands, but it will take a while more and won't be possible to uplift.
Flags: needinfo?(bvandyk)
Yes, I'll have a look. May take some time to familiarise myself with this region of the code, but I'll reach out should I need help.
Flags: needinfo?(bvandyk)
Assignee: nobody → bvandyk
For the direct listeners, I think we're going to need to rewrite sample timestamps somewhere in order to accommodate pauses (timestamp = timestamp - pausedTime). I'm thinking the MediaEncoder can do this. However, I'd like to see 1356054 resolved before this, otherwise we're going to need to have special handling based on the source of the samples.
Depends on: 1356054
Actually, the above may be incorrect since the timestamps are always in usecs, and the duration is the inconsistent part. I'm going to see what can be done.

How were we handling this prechange? I've had a look at the patches but am not sure I've located the relevant part. Was the MSG rewriting time stamps in the case of a resume following a pause?
(In reply to Bryce Van Dyk (:SingingTree) from comment #5)
> Actually, the above may be incorrect since the timestamps are always in
> usecs, and the duration is the inconsistent part. I'm going to see what can
> be done.
> 
> How were we handling this prechange? I've had a look at the patches but am
> not sure I've located the relevant part. Was the MSG rewriting time stamps
> in the case of a resume following a pause?

MediaEncoder would block media while suspended, see [1].

Then on resume, TrackEncoder would pass the duration of new media to the encoder. Since no data was fed to TrackEncoder during pause, this would be fine.

I'd guess that this regressed when video was changed from durations to timestamps, as the VideoTrackEncoder doesn't account for pause, and timestamps inherently cover the suspended period. (Do we get video freeze for the duration of the pause? Then that holds.)

Quickest fix is probably to notify VideoTrackEncoder about Pause and Resume so it can update the timestamp of mLastChunk accordingly. Fine tuning sync is a bigger issue but bug 1296531 will fix that.


[1] http://searchfox.org/mozilla-central/rev/068e6f292503df13515a52321fc3844e237bf6a9/dom/media/encoder/MediaEncoder.cpp#52
Can anyone confirm that the resume functionality is broken here for Video + Audio? I'm seeing Audio pause, but does not resume.

Fiddle with resume:
https://jsfiddle.net/SingingTree/psctg12c/

I've got a WIP fix that handles this in MediaEncoder at muxing time for Video (resume also works). However Audio is still not resuming. I'm going to look at if the functionality can be moved to the encoder side as noted in #6, as well as why Audio is unhappy.
Confirmed that audio resume broke at the same time (same regression range, on windows this time, which somehow finds a lot narrower range):

2017-05-01T10:45:27: INFO : Narrowed inbound regression window from [79415ce5, 4a3775a4] (3 revisions) to [79415ce5, a02925fe] (2 revisions) (~1 steps left)
2017-05-01T10:45:27: DEBUG : Starting merge handling...
2017-05-01T10:45:27: DEBUG : Using url: https://hg.mozilla.org/integration/mozilla-inbound/json-pushes?changeset=a02925fe4ded04e2f5523fbd7e5ddfa46b190d39&full=1
2017-05-01T10:45:27: DEBUG : Found commit message:
Bug 1201363 - Do not copy video segment to StreamTracks in TrackUnionStream. r=jesup

Now everything is ready. We can make NotifyQueuedTrackChanges only triggered by TRACK_EVENT_CREATED and TRACK_EVENT_ENDED without breaking anything. Also we make TrackUnionStream no longer copying data in video case.

MozReview-Commit-ID: IgLx1mpBWB3

2017-05-01T10:45:27: INFO : The bisection is done.
2017-05-01T10:45:27: INFO : Stopped

Note that if I remove video from the gUM call, then audio resumes fine.
Comment on attachment 8865285 [details]
Bug 1354457 - Stop video frames being forwarded for direct connections when recorder is suspended.

https://reviewboard.mozilla.org/r/136926/#review139936

::: dom/media/encoder/MediaEncoder.h:23
(Diff revision 1)
> -class MediaStreamVideoRecorderSink : public MediaStreamVideoSink
> +  class MediaStreamVideoRecorderSink : public MediaStreamVideoSink
> -{
> +  {

Indentation
Attachment #8865285 - Flags: review?(pehrson) → review+
Comment on attachment 8865286 [details]
Bug 1354457 - Remove old suspend logic from media encoder.

https://reviewboard.mozilla.org/r/136928/#review139938

::: dom/media/encoder/MediaEncoder.cpp:55
(Diff revision 1)
>                                   TrackID aID,
>                                   StreamTime aTrackOffset,
>                                   uint32_t aTrackEvents,
>                                   const MediaSegment& aRealtimeMedia)
>  {
> -  if (mSuspended == RECORD_NOT_SUSPENDED) {
> +  if (!mSuspended) {

Could you change this to an early-exit guard while you're at it?
Attachment #8865286 - Flags: review?(pehrson) → review+
Comment on attachment 8865287 [details]
Bug 1354457 - Record time spent paused in media encoder and adjust time stamps when muxing.

https://reviewboard.mozilla.org/r/136930/#review139940

Mostly minor issues, but enough of them to warrant another round.

::: dom/media/encoder/MediaEncoder.h:17
(Diff revision 1)
>  #include "CubebUtils.h"
>  #include "MediaStreamGraph.h"
>  #include "MediaStreamListener.h"
>  #include "nsAutoPtr.h"
>  #include "MediaStreamVideoSink.h"
> +#include "TimeUnits.h"

It would be really lovely to have this include in the cpp instead. Hopefully it's as simple as moving the definitions of Suspend and Resume?

::: dom/media/encoder/MediaEncoder.h:128
(Diff revision 1)
> +    if (mSuspended) {
> +      media::TimeUnit timeSpentPaused =
> +        media::TimeUnit::FromTimeDuration(
> +          TimeStamp::Now() - mLastPauseStartTime);
> +      MOZ_ASSERT(timeSpentPaused.ToMicroseconds() >= 0);
> +      mMicrosecondsSpentPaused += timeSpentPaused.ToMicroseconds();
> +    }
>      mSuspended = false;

Two atomics don't make an atomic. Are we at risk of races here?

::: dom/media/encoder/MediaEncoder.h:128
(Diff revision 1)
> -   * Arm to collect the Duration of the next video frame and give it
> +   * Calculates time spent paused in order to offset frames. */
> -   * to the next frame, in order to avoid any possible loss of sync. */
>    void Resume()
>    {
>      MOZ_ASSERT(NS_IsMainThread());
> +    if (mSuspended) {

Making this an early-exit guard would be great.

::: dom/media/encoder/MediaEncoder.h:133
(Diff revision 1)
> +    if (mSuspended) {
> +      media::TimeUnit timeSpentPaused =
> +        media::TimeUnit::FromTimeDuration(
> +          TimeStamp::Now() - mLastPauseStartTime);
> +      MOZ_ASSERT(timeSpentPaused.ToMicroseconds() >= 0);
> +      mMicrosecondsSpentPaused += timeSpentPaused.ToMicroseconds();

IMO we should handle overflow errors from the CheckedInt storage in prod as well. Would the best way be to
```
mMicrosecondsSpentPaused = (TimeUnit(mMicrosecondsSpentPaused)
                           + timeSpentPaused).ToMicroseconds());
MOZ_RELEASE_ASSERT(mMicrosecondsSpentPaused.IsValid());
```
here or would there be a reason to handle it gracefully?

::: dom/media/encoder/MediaEncoder.h:260
(Diff revision 1)
>    bool mShutdown;
>    bool mDirectConnected;
> +  // Tracks if the encoder is suspended (paused). Used on the main thread and
> +  // MediaRecorder's read thread.
>    Atomic<bool> mSuspended;
> +  // Time stamp of when the last happened. Should only be accessed on the

nits:
s/Time stamp/Timestamp/
s/last/last pause/

::: dom/media/encoder/MediaEncoder.cpp:329
(Diff revision 1)
> +    if (frame->GetTimeStamp() > mMicrosecondsSpentPaused &&
> +        frame->GetTimeStamp() - mMicrosecondsSpentPaused > mLastMuxedTimestamp) {
> +      frame->SetTimeStamp(frame->GetTimeStamp() - mMicrosecondsSpentPaused);

This reads an atomic many times. Should probably take a copy first instead.

::: dom/media/encoder/MediaEncoder.cpp:335
(Diff revision 1)
> +        frame->GetTimeStamp() - mMicrosecondsSpentPaused > mLastMuxedTimestamp) {
> +      frame->SetTimeStamp(frame->GetTimeStamp() - mMicrosecondsSpentPaused);
> +    } else {
> +      frame->SetTimeStamp(mLastMuxedTimestamp);
> +    }
> +    MOZ_ASSERT(mLastMuxedTimestamp <= frame->GetTimeStamp());

I'm a bit wary of lone `MOZ_ASSERT`s like this (been bitten a number of times in non-debug builds).

In prod, should this be something like `mLastMuxedTimestamp = std::max(mLastMuxedTimestamp, frame->GetTimeStamp());`?

I often put the assert condition in an if and hardode a `MOZ_ASSERT(false)` so that the failure path is explicit and clear.
Attachment #8865287 - Flags: review?(pehrson)
Comment on attachment 8865287 [details]
Bug 1354457 - Record time spent paused in media encoder and adjust time stamps when muxing.

https://reviewboard.mozilla.org/r/136930/#review140286

::: dom/media/encoder/MediaEncoder.h:128
(Diff revision 1)
> +    if (mSuspended) {
> +      media::TimeUnit timeSpentPaused =
> +        media::TimeUnit::FromTimeDuration(
> +          TimeStamp::Now() - mLastPauseStartTime);
> +      MOZ_ASSERT(timeSpentPaused.ToMicroseconds() >= 0);
> +      mMicrosecondsSpentPaused += timeSpentPaused.ToMicroseconds();
> +    }
>      mSuspended = false;

We've discussed this on IRC, I believe the outcome was to leave this as is, is that correct?

::: dom/media/encoder/MediaEncoder.h:133
(Diff revision 1)
> +    if (mSuspended) {
> +      media::TimeUnit timeSpentPaused =
> +        media::TimeUnit::FromTimeDuration(
> +          TimeStamp::Now() - mLastPauseStartTime);
> +      MOZ_ASSERT(timeSpentPaused.ToMicroseconds() >= 0);
> +      mMicrosecondsSpentPaused += timeSpentPaused.ToMicroseconds();

I came up with a solution involving CheckInts, but some of my colleagues suggested that if timeSpentPaused is non negative, then an extra check here is overkill.

I'm of two minds, however if the TimeStamp interface can be trusted I don't think we can get an overflow here within any feasible time frame.

For now I'll upgrade the above assert to a release assert, as that's the only way I can see this breaking.

Thoguhts?

::: dom/media/encoder/MediaEncoder.cpp:335
(Diff revision 1)
> +        frame->GetTimeStamp() - mMicrosecondsSpentPaused > mLastMuxedTimestamp) {
> +      frame->SetTimeStamp(frame->GetTimeStamp() - mMicrosecondsSpentPaused);
> +    } else {
> +      frame->SetTimeStamp(mLastMuxedTimestamp);
> +    }
> +    MOZ_ASSERT(mLastMuxedTimestamp <= frame->GetTimeStamp());

In this case I think it's impossible to hit this assert, I stuck it there to document world state after the if. The timestamp on our current frame has to either set such that is greater than the previous time stamp, or be the same. I think the outcome is the same as the std::max expression, let me know if I'm looking at this wrong. However, it seems like the code could be made more readable?

I've rewritten the code for the next update. Let me know if you think the new version is any better or not.
Comment on attachment 8865287 [details]
Bug 1354457 - Record time spent paused in media encoder and adjust time stamps when muxing.

https://reviewboard.mozilla.org/r/136930/#review140286

> We've discussed this on IRC, I believe the outcome was to leave this as is, is that correct?

Yes.

With the rationale that `mMicrosecondsSpentPaused` is only written when `mSuspended` is `true`, and only read when `mSuspended` is `false`. Hard to enforce wrt future changes, but temporary until bug 1296531 lands so OK.

> I came up with a solution involving CheckInts, but some of my colleagues suggested that if timeSpentPaused is non negative, then an extra check here is overkill.
> 
> I'm of two minds, however if the TimeStamp interface can be trusted I don't think we can get an overflow here within any feasible time frame.
> 
> For now I'll upgrade the above assert to a release assert, as that's the only way I can see this breaking.
> 
> Thoguhts?

TimeUnit uses CheckedInt64 underneath, so why not a simple IsValid() check? IMHO put that in the release assert, and the non-negative check in a regular assert. The former would take a very very long time to hit but the latter would be more random, so more likely to show up in tests.

> In this case I think it's impossible to hit this assert, I stuck it there to document world state after the if. The timestamp on our current frame has to either set such that is greater than the previous time stamp, or be the same. I think the outcome is the same as the std::max expression, let me know if I'm looking at this wrong. However, it seems like the code could be made more readable?
> 
> I've rewritten the code for the next update. Let me know if you think the new version is any better or not.

Looks alright. For readability I think you can skip the intermediary vars (if the types are right), inline it all, and add an else for the alternative path. Perhaps a comment on the alternative path and what is its purpose in life.
Comment on attachment 8865287 [details]
Bug 1354457 - Record time spent paused in media encoder and adjust time stamps when muxing.

https://reviewboard.mozilla.org/r/136930/#review140286

> TimeUnit uses CheckedInt64 underneath, so why not a simple IsValid() check? IMHO put that in the release assert, and the non-negative check in a regular assert. The former would take a very very long time to hit but the latter would be more random, so more likely to show up in tests.

I've added extra checks, it's a bit more verbose as TimeUnit internally uses a checked i64, while we eventually want to end up in a checked u64. The other pain point here is that Atomic and CheckedInt do not play nice, so we need to move out of the atomic by casting (or assignment to its base type).

> Looks alright. For readability I think you can skip the intermediary vars (if the types are right), inline it all, and add an else for the alternative path. Perhaps a comment on the alternative path and what is its purpose in life.

I've set this back to something more like the original: have an explicit else, and no inlining. Added comments, with particular focus on the else.
Comment on attachment 8865287 [details]
Bug 1354457 - Record time spent paused in media encoder and adjust time stamps when muxing.

https://reviewboard.mozilla.org/r/136930/#review140488

::: dom/media/encoder/MediaEncoder.cpp:64
(Diff revisions 2 - 3)
> -  mMicrosecondsSpentPaused += timeSpentPaused.ToMicroseconds();
> +  CheckedUint64 totalMicrosecondsPaused = timeSpentPaused.ToMicroseconds();
> +  totalMicrosecondsPaused += static_cast<uint64_t>(mMicrosecondsSpentPaused);
> +  MOZ_RELEASE_ASSERT(totalMicrosecondsPaused.isValid());

I meant to just check the validity of `timeSpentPaused` with `TimeUnit::IsValid`.

::: dom/media/encoder/MediaEncoder.cpp:373
(Diff revisions 2 - 3)
> -    frame->SetTimeStamp(newTimestamp);
> +    // No matter what our frames should be ordered by the above!
>      MOZ_ASSERT(mLastMuxedTimestamp <= frame->GetTimeStamp());

This comment would probably be better as an arg to the assert. Or if you drop it, that's fine too.
Attachment #8865287 - Flags: review?(pehrson) → review+
Comment on attachment 8867591 [details]
Bug 1354457 - Add test to check video is paused and resumed correctly in MediaRecorder.

https://reviewboard.mozilla.org/r/139144/#review142432

Mostly nits. FWIW I think we should land a resume test as well. Up to you if you want to add it to this test or make another. I can help you get the proper guarantees should you have races. Let's chat on IRC.

::: dom/media/test/test_mediarecorder_pause_video.html:4
(Diff revision 1)
> +<!DOCTYPE HTML>
> +<html>
> +<head>
> +  <title>Test MediaRecorder Recording pause</title>

suggestion: "not recording during pause"

::: dom/media/test/test_mediarecorder_pause_video.html:23
(Diff revision 1)
> +
> +  var canvas_size = 100;
> +  var new_canvas_size = 50;
> +
> +  canvas.width = canvas.height = canvas_size;
> +  document.getElementById("content").appendChild(canvas);

Since your content is static you could stick the canvas and the video element into the html directly.

::: dom/media/test/test_mediarecorder_pause_video.html:57
(Diff revision 1)
> +  mediaRecorder.onstart = () => {
> +    info("Got 'start' event");
> +    // We just want one frame encoded before we pause
> +    mediaRecorder.pause();
> +    // We may rewrite this once we settle Bug 1363915, could listen for pause event instead
> +    is(mediaRecorder.state, 'paused', 'Media recorder should be paused');

Should we check the state after `stop()`/in `onstop` as well?

::: dom/media/test/test_mediarecorder_pause_video.html:65
(Diff revision 1)
> +    function draw(timestamp) {
> +        numberOfPaintsSincePause++;
> +        if(numberOfPaintsSincePause > 60) {
> +            mediaRecorder.stop();
> +        } else {
> +            window.requestAnimationFrame(draw);
> +        }
> +    }

nit: Indentation (does your editor know we use 2 spaces? :-) )

And I think it's a bit bad form to inline a function like this. You could inline a lambda instead like

```
let draw = timestamp => {
  ...
};
```

::: dom/media/test/test_mediarecorder_pause_video.html:89
(Diff revision 1)
> +        ok(helper.isPixel(helper.getPixel(video), helper.red, 128), "Last frame should be red");
> +        SimpleTest.finish();

Indentation

::: dom/media/test/test_mediarecorder_pause_video.html:91
(Diff revision 1)
> +      SimpleTest.finish();
> +    };
> +    video.onended = e => {
> +        ok(helper.isPixel(helper.getPixel(video), helper.red, 128), "Last frame should be red");
> +        SimpleTest.finish();
> +    }

missing semicolon

::: dom/media/test/test_mediarecorder_pause_video.html:92
(Diff revision 1)
> +    };
> +    video.onended = e => {
> +        ok(helper.isPixel(helper.getPixel(video), helper.red, 128), "Last frame should be red");
> +        SimpleTest.finish();
> +    }
> +    // The video will resize once it loads its metadata, only listen for resizes after that

Also check that the size is equal to `canvas_size` here.

::: dom/media/test/test_mediarecorder_pause_video.html:94
(Diff revision 1)
> +        // We shouldn't have any resize events once the video is loaded
> +        video.onresize = e => {
> +        ok(false, "Should not have any resize events!");
> +        }

Indentation

::: dom/media/test/test_mediarecorder_pause_video.html:98
(Diff revision 1)
> +    video.onloadedmetadata = e => {
> +        // We shouldn't have any resize events once the video is loaded
> +        video.onresize = e => {
> +        ok(false, "Should not have any resize events!");
> +        }
> +    }

missing semicolon

::: dom/media/test/test_mediarecorder_pause_video.html:100
(Diff revision 1)
> +    helper.waitForPixelColor(video, helper.blue, 128, "Look for blue (we do not want to see blue)")
> +      .then(() => {
> +        ok(false, "Should not have recorded any blue frames!");
> +      });

I think `waitForPixelColorTimeout` could be more suiting here. Most importantly though, don't leave it dangling.

::: dom/media/test/test_mediarecorder_pause_video.html:114
(Diff revision 1)
> +  mediaRecorder.start();
> +  is(mediaRecorder.state, "recording", "Media recorder should be recording");
> +}
> +
> +SimpleTest.waitForExplicitFinish();
> +SpecialPowers.pushPrefEnv({set:[["media.recorder.max_memory", 1]]}, startTest);

Is this necessary?
Attachment #8867591 - Flags: review?(pehrson) → review+
Comment on attachment 8867591 [details]
Bug 1354457 - Add test to check video is paused and resumed correctly in MediaRecorder.

https://reviewboard.mozilla.org/r/139144/#review142432

Fixed the points raised here and have added resume functionality. Currently working on adding audio as well. Will update the review when I have that sorted.

> nit: Indentation (does your editor know we use 2 spaces? :-) )
> 
> And I think it's a bit bad form to inline a function like this. You could inline a lambda instead like
> 
> ```
> let draw = timestamp => {
>   ...
> };
> ```

It should know, it's just been feeling a bit under the weather lately :S Updated the inner function to arrow notation (pulled the original convention for the downsize resolution test).

> I think `waitForPixelColorTimeout` could be more suiting here. Most importantly though, don't leave it dangling.

I'll rework this to check to happen as part of timeupdate, this avoids dangling promises and usage of timers.

> Is this necessary?

No, I had copied this from one of the another test under the assumption that it was in someway useful or important for the recorder tests. Will remove it.
Comment on attachment 8867591 [details]
Bug 1354457 - Add test to check video is paused and resumed correctly in MediaRecorder.

https://reviewboard.mozilla.org/r/139144/#review142432

> It should know, it's just been feeling a bit under the weather lately :S Updated the inner function to arrow notation (pulled the original convention for the downsize resolution test).

I've run insto some issues with the AV case (swapping audio tracks in a stream is killing the recorder), so pushing updates to this while working out those problems.
Bug 1365759 raised as a follow up for more detailed testing.
Comment on attachment 8867591 [details]
Bug 1354457 - Add test to check video is paused and resumed correctly in MediaRecorder.

https://reviewboard.mozilla.org/r/139144/#review144012

::: dom/media/test/test_mediarecorder_pause_resume_video.html:62
(Diff revision 2)
> +    is(mediaRecorder.state, 'paused', 'Media recorder should be paused');
> +    // Change our canvas color and size, these changes should not be recorded due to pause
> +    canvas.width = canvas.height = new_canvas_size;
> +    helper.drawColor(canvas, helper.blue);
> +
> +    // Make sure we get some more blue paints before we resume. Then change color to green and

There's no drawing of blue happening in `draw()`, unlike your comment indicates.

::: dom/media/test/test_mediarecorder_pause_resume_video.html:63
(Diff revision 2)
> +    // resume and record green.  This is a kludge, ideally we'd have an event on the recorder to
> +    // say it has received data, but no such event exists.

I disagree. It would be impractical to have a per-frame event on the recorder. The "pause" and "resume" events would be fine though.
Pushed by bvandyk@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/09b32ef21528
Stop video frames being forwarded for direct connections when recorder is suspended. r=pehrsons
https://hg.mozilla.org/integration/autoland/rev/a17bc9ce68bc
Remove old suspend logic from media encoder. r=pehrsons
https://hg.mozilla.org/integration/autoland/rev/782b0c34f43b
Record time spent paused in media encoder and adjust time stamps when muxing. r=pehrsons
https://hg.mozilla.org/integration/autoland/rev/3e577500606b
Add test to check video is paused and resumed correctly in MediaRecorder. r=pehrsons
Blocks: 1201363
Flags: in-testsuite+
Please request uplift on this when you get a chance.  Thanks!
Flags: needinfo?(bvandyk)
Comment on attachment 8865285 [details]
Bug 1354457 - Stop video frames being forwarded for direct connections when recorder is suspended.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1201363 appears to have lead to this regression.
[User impact if declined]: Media recorder pause and resume functionality remains broken. This results in issues such as video not pausing, and audio not resuming from pauses.
[Is this code covered by automated tests?]: Not prior to Bug 1354457, however I have introduced a test as part of this bug. The test covers pause and resume, only for video recordings.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: Yes, it would be good to verify. The following steps can be used:
1) Open the fiddle at: https://jsfiddle.net/SingingTree/4jurzdxq/
2) Begin recording both audio and video (the fiddle will record for 5 seconds)
3) Pause the recording, then resume
4) The expected result is that the recording will play the audio and video from before the pause, and after the resume. Video and audio should not be recorded during the pause. Without this fix, the video will not pause and the audio will not resume.

These steps can be repeated with https://jsfiddle.net/SingingTree/y09xLe31/ to ensure audio only recording also works. As well as https://jsfiddle.net/SingingTree/7233ytej/ to ensure video only works.
[List of other uplifts needed for the feature/fix]: The 2 patches following this one on bug 1354457 are code changes needed to address this. The 4th introduces an automated test.
[Is the change risky?]: I believe this is medium risk.
[Why is the change risky/not risky?]: This is moderate sized code change. It addresses functionality which is already busted, though it is possible it could regress pause/resume in other ways, or regress other media recorder functionality. I would not expect regressions outside of media recorder. This has not had long to bake in nightly, additional manual testing could help mitigate this.
[String changes made/needed]: None.
Flags: needinfo?(bvandyk)
Attachment #8865285 - Flags: approval-mozilla-beta?
Comment on attachment 8865285 [details]
Bug 1354457 - Stop video frames being forwarded for direct connections when recorder is suspended.

Since this issue has been there for a while (from 51),  the fix has not been baked long enough in nightly and we are in late beta. I would like this ride the train to reduce the risk. Beta54- and mark 54 won't fix.
Attachment #8865285 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Doubtful this meets the criteria for ESR52 inclusion also. Feel free to set it back to affected and nominate for approval if you feel otherwise.
Depends on: 1458852
You need to log in before you can comment on or make changes to this bug.