Closed Bug 1382718 Opened 7 years ago Closed 7 years ago

MediaRecorder throws RecordErrorEvent on valid window mediaStream with no further info

Categories

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

defect

Tracking

()

VERIFIED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: sam, Assigned: bryce)

Details

Attachments

(5 files)

Attached video ff-bug.mp4
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:54.0) Gecko/20100101 Firefox/54.0
Build ID: 20170628075643

Steps to reproduce:

1. Go to URL: https://bugreplay-site-dev.appspot.com/extension/ff
2. Click start
3. Pick a window to share.
4. Open developer console


Actual results:

Immediately on start(), the MediaRecorder throws a generic error and stops.


Expected results:

It should record and not fire the onerror.
This fails if I pick:
- The terminal of my OSX machine
- A Firefox window

It works for normal OSX windows, such as the "About this mac" window.

It fails exactly here: http://searchfox.org/mozilla-central/source/dom/media/encoder/VP8TrackEncoder.cpp#476

Attached is the value of *data when we fail, from lldb.
Bryce, want to have a look? Quite easy to repro using the instruction in comment 0 (at least here on OSX).
Flags: needinfo?(bvandyk)
Status: UNCONFIRMED → NEW
Rank: 17
Ever confirmed: true
Priority: -- → P1
Relevant:  https://bugzilla.mozilla.org/show_bug.cgi?id=1382718 (Media Recording - Errors fired from an onerror callback always fire GenericError, which provide no context of the problem)
+baku in case this is a Principal issue...
-baku since I just noticed comment 1 indicates it's a format-of-the-buffer issue
(In reply to Paul Adenot (:padenot) from comment #2)
> Created attachment 8888434 [details]
> *data value when we fail, from lldb
> 
> Bryce, want to have a look? Quite easy to repro using the instruction in
> comment 0 (at least here on OSX).

How come it doesn't fall into the code at line 401?  I.e. planar 420 with !mCbSkip.  Can you break on 389 and see what it does?  Is it not planar420?
I can repro on Windows 10 also. Will investigate.
Assignee: nobody → bvandyk
Flags: needinfo?(bvandyk)
Here's a fiddle to mess with and repro the same issue: https://jsfiddle.net/SingingTree/h2m3teu1/

It looks like the data being given to the encoders has dimensions which are resulting in not matching any formats. When capturing a 640, 480 video (as specified in the constraints given to getUserMedia), I get images with Y dimensions width=729 height=427 and CbCr dimensions width=365 height=214. These are different than :padenot's values (the original faulting page appears to request 640,480), but both sets of values will result in the same issue.

> How come it doesn't fall into the code at line 401?  I.e. planar 420 with !mCbSkip.  Can you break on 389 and see what it does?  > Is it not planar420?

It looks like we're getting off by ones for the check. In my case ( CbCrDims * 2 = 730, 428 ) != ( YDims = 729, 427 ). Messing with the dimensions in the fiddle seems to always result in this off by one in at least one of the dimensions.

I'm unfamiliar with the code doing the capture of the windows, so would appreciate any thoughts as to what's going on (size is different than requested size, wonky 420 values). I am currently looking around here: https://dxr.mozilla.org/mozilla-central/source/media/webrtc/trunk/webrtc/video_engine/desktop_capture_impl.cc#488 It appears at this point the dimensions are already set.
If I skip or revise the check here http://searchfox.org/mozilla-central/source/dom/media/encoder/VP8TrackEncoder.cpp#399 I'm able to get a recording. I'm now thinking that the issue is how this check is being done. I believe the correct check should be something like:

`return (aData->mYSize.height + 1) / 2 == aData->mCbCrSize.height && (aData->mYSize.width + 1) / 2 == aData->mCbCrSize.width;`

or just straight calling into ImageUtils which has code like the above. I'll create a patch as such.

I'm still uncertain about the dimensions coming back different than the gUM args, but that seems to be a red herring.
fwiw I was having the same problem without any dimensions specified. I only started specifying dimensions when I was trying different things to get it not to error.
It does sound like that check is off, though I'm not an expert on the Image code.

for gum capture, constraints generally do not exactly set the final size, they merely request something near it.  (Constraints language is a complex beast...).
Comment on attachment 8889232 [details]
Bug 1382718 - Expose ImageUtils.h so utils can be used more widely.

https://reviewboard.mozilla.org/r/160286/#review166542
Attachment #8889232 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8889669 [details]
Bug 1382718 - Update video encoder gtest to use appropriate image objects.

https://reviewboard.mozilla.org/r/160734/#review167180
Attachment #8889669 - Flags: review?(rjesup) → review+
Comment on attachment 8889233 [details]
Bug 1382718 - Update detection of image bitmap format in VP8 Encoder.

https://reviewboard.mozilla.org/r/160288/#review167184
Attachment #8889233 - Flags: review?(rjesup) → review+
Pushed by bvandyk@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/000f58399e4f
Expose ImageUtils.h so utils can be used more widely. r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/10d202bedc9d
Update detection of image bitmap format in VP8 Encoder. r=jesup
https://hg.mozilla.org/integration/autoland/rev/d55e2461a61c
Update video encoder gtest to use appropriate image objects. r=jesup
Awesome, thanks again for working on this guys, I really appreciate it. I just downloaded Firefox Nightly and was still seeing the same issue, that's expected right?
When would I be able to test this in nightly?
You're most welcome. Nightly updates at around 12:00PM UTC. I'd expect this to be reflected in the next update which is 14 hours-ish from now (provided my timezone math isn't failing me). If this isn't fixed then, let me know and I'll take another look.
Worked for me, thanks again!
Status: RESOLVED → VERIFIED
(not sure if i'm supposed to verify but the button was there)
You need to log in before you can comment on or make changes to this bug.