1424191 - MediaRecorder(videoStream) unexpectedly stops itself

Status: RESOLVED FIXED

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

Description

[Kaiido]

Attachment: MediaRecorderBug.html

When using the MediaRecorder with an LocalMediaStream fed by a VideoTrack, the MediaRecorder stops itself unexpectedly.

The dataavailable event fires, but with an empty Blob as .data .

The LocalMediaStream still reports has active.

Apparently caused by bug#1388219


mozregression results: 

Last good revision: 574f4f58fe09dd590ea892406e237318c31705b4 (2017-12-03)
First bad revision: 18a9cb5cb32d0e8031d0a80901b199d5e9827d83 (2017-12-05)

Last good revision: bdb599143009e2facf0306169f729e679729d493
First bad revision: 4a003542df78bc7f08b37c4759e3a64c71e74552

Last good revision: 1dd65ba14e0f0f67f6660273949c3a81a4cd1fa8
INFO: First bad revision: 741eed8ed628c81dec7e8d2b038f641591c71f9b
INFO: Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=1dd65ba14e0f0f67f6660273949c3a81a4cd1fa8&tochange=741eed8ed628c81dec7e8d2b038f641591c71f9b

Comment 1

[Kaiido]

So after I read what incriminated bug#1388219 did, I found that setting a delay after the stream has started makes the MediaRecorder happy. 

So I guess the problem is that the stream is available before the constraints get found, and causes the stream to somehow switch between constraints, while being recorded?

Though I'm not sure about this, since even when setting the constraints to exact values the issue is still there.

Comment 2

[Andreas Pehrson [:pehrsons]]

This is the same as we were discussing on IRC last week, right?
If so, this is due to a video frame reaching the recorder with either a real frame of resolution 0x0 or metadata for "intrinsic size" of 0x0. It seems to happen on the first real frame that comes from our video capture backend.

[Tracking Requested - why for this release]:
Regression that makes a very common case of recording a video track throw an error.

Comment 3

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Yes. I think this is what florian encountered.

Comment 4

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

Tracking for 59, since it sounds like this is a recent regression that may affect many users.

Comment 5

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Attachment: Bug 1424191 - set mImages[i] to nullptr in MediaEngineRemoteVideoSource::Start().

Review commit: https://reviewboard.mozilla.org/r/207166/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/207166/

Comment 6

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Comment on attachment 8936454 [details]
Bug 1424191 - set mImages[i] to nullptr in MediaEngineRemoteVideoSource::Start().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/207166/diff/1-2/

Comment 7

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=726e84b0ddba436cdf5370a79e8e7144fe01a086

Comment 8

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8936454 [details]
Bug 1424191 - set mImages[i] to nullptr in MediaEngineRemoteVideoSource::Start().

https://reviewboard.mozilla.org/r/207166/#review213422

::: dom/media/webrtc/MediaEngineCameraVideoSource.cpp:33
(Diff revision 2)
> +  // If we sent an Image object with 0 size, it will cause encoder init check failure.
> +  if (size.width == 0 || size.height == 0) {
> +    image = nullptr;
> +  }

The issue here is that we pass along a real image with resolution (0,0), isn't it?

When we append a non-null image it should have non-zero width and height. Making sure they're non-zero would be the proper fix for this.

(I don't understand how image->GetSize() returns (0,0) - how is that happening? Scaling goes wrong?)

Comment 9

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Here is the behavior before any of multi-e10s patches landed:
Before any MediaEngineRemoteVideoSource::DeliverFrame() is called by Camera IPC thread, mImage is null [1]
We keep calling AppendToTrack with null mImage [2]
Since image is null, we set the size to 0 [3]
[1] https://hg.mozilla.org/mozilla-central/file/91fc3a79606b/dom/media/webrtc/MediaEngineRemoteVideoSource.cpp#l436
[2] https://hg.mozilla.org/mozilla-central/file/91fc3a79606b/dom/media/webrtc/MediaEngineRemoteVideoSource.cpp#l357
[3] https://hg.mozilla.org/mozilla-central/file/91fc3a79606b/dom/media/webrtc/MediaEngineCameraVideoSource.cpp#l31

I am not expert of MSG, but according to my observation, in this period (before the first DeliverFrame being called),
If we call AppendToTrack with 0 size and non-null image, there will be encoder init error.
If we return directly when the size is 0, it will cause audio latency.
The only way which works is to keep feeding 0 size and null image to MSG.

Back to the latest behavior of my WIP.
Now we will set an empty image to mImages[i] in MediaEngineRemoteVideoSource::Start() [4]
So before any DeliverFrame() is called, mImages[i] is not null already. In default, GetSize() will return 0x0.
So in this period, we will keep feeding non-null image with 0 size, which causes the encoder init failure.

[4] https://searchfox.org/mozilla-central/rev/b0098afaeaad24a6752aa418ca3e86eade5fab17/dom/media/webrtc/MediaEngineRemoteVideoSource.cpp#193

Comment 10

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

s/Back to the latest behavior of my WIP/Back to the behavior of latest nightly/

Comment 11

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Comment on attachment 8936454 [details]
Bug 1424191 - set mImages[i] to nullptr in MediaEngineRemoteVideoSource::Start().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/207166/diff/2-3/

Comment 12

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8936454 [details]
Bug 1424191 - set mImages[i] to nullptr in MediaEngineRemoteVideoSource::Start().

https://reviewboard.mozilla.org/r/207166/#review213640

Looks good! Thanks.

::: dom/media/webrtc/MediaEngineCameraVideoSource.cpp:31
(Diff revision 3)
> +  // Before the first DeliverFrame() arrives, we need to keep sending
> +  // null image with size 0x0 to avoid blocking audio/video track.

nit: I think this is quite self-explained.

Comment 13

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Comment on attachment 8936454 [details]
Bug 1424191 - set mImages[i] to nullptr in MediaEngineRemoteVideoSource::Start().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/207166/diff/3-4/

Comment 14

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=81fb0b4a4fd6dae084bb07a7697dd22582e8810e&selectedJob=151787525

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bb8aa824b37b
set mImages[i] to nullptr in MediaEngineRemoteVideoSource::Start(). r=pehrsons

Comment 16

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/bb8aa824b37b