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)
Core
Audio/Video: Recording
Tracking
()
VERIFIED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: sam, Assigned: bryce)
Details
Attachments
(5 files)
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.
Comment 1•7 years ago
|
||
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.
Comment 2•7 years ago
|
||
Bryce, want to have a look? Quite easy to repro using the instruction in comment 0 (at least here on OSX).
Flags: needinfo?(bvandyk)
Updated•7 years ago
|
Status: UNCONFIRMED → NEW
Rank: 17
Ever confirmed: true
Priority: -- → P1
Reporter | ||
Comment 3•7 years ago
|
||
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)
Comment 4•7 years ago
|
||
+baku in case this is a Principal issue...
Comment 5•7 years ago
|
||
-baku since I just noticed comment 1 indicates it's a format-of-the-buffer issue
Comment 6•7 years ago
|
||
(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?
Assignee | ||
Comment 7•7 years ago
|
||
I can repro on Windows 10 also. Will investigate.
Assignee: nobody → bvandyk
Flags: needinfo?(bvandyk)
Assignee | ||
Comment 8•7 years ago
|
||
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.
Assignee | ||
Comment 9•7 years ago
|
||
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.
Reporter | ||
Comment 10•7 years ago
|
||
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.
Comment 11•7 years ago
|
||
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 20•7 years ago
|
||
mozreview-review |
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 21•7 years ago
|
||
mozreview-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 22•7 years ago
|
||
mozreview-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+
Comment 23•7 years ago
|
||
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
Comment 24•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/000f58399e4f https://hg.mozilla.org/mozilla-central/rev/10d202bedc9d https://hg.mozilla.org/mozilla-central/rev/d55e2461a61c
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Reporter | ||
Comment 25•7 years ago
|
||
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?
Assignee | ||
Comment 26•7 years ago
|
||
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.
Reporter | ||
Comment 27•7 years ago
|
||
Worked for me, thanks again!
Reporter | ||
Updated•7 years ago
|
Status: RESOLVED → VERIFIED
Reporter | ||
Comment 28•7 years ago
|
||
(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.
Description
•