Closed
Bug 1182426
Opened 9 years ago
Closed 9 years ago
Make MediaRecorder support all image formats
Categories
(Core :: Audio/Video: Recording, defect)
Core
Audio/Video: Recording
Tracking
()
RESOLVED
FIXED
mozilla44
Tracking | Status | |
---|---|---|
firefox44 | --- | fixed |
People
(Reporter: pehrsons, Assigned: pehrsons)
References
Details
Attachments
(8 files)
(deleted),
text/x-review-board-request
|
roc
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
roc
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
roc
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
pehrsons
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
roc
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
roc
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
roc
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
roc
:
review+
|
Details |
IMO MediaRecorder should at least support all streams you can get from APIs in gecko. Examples of failing ones are canvas.captureStream() streams and video.mozCaptureStream() streams depending on what the decoder generates.
This is because VP8TrackEncoder locks itself to PlanarYCbCr images: https://hg.mozilla.org/mozilla-central/file/e7e69cc8c07b/dom/media/encoder/VP8TrackEncoder.cpp#l261
We could convert other images similarly to what MediaPipeline does: https://hg.mozilla.org/mozilla-central/file/e7e69cc8c07b/media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp#l1194
Assignee | ||
Comment 1•9 years ago
|
||
Bug 1182426 - Part 1. Sort includes in VP8TrackEncoder.cpp alphabetically. r?roc
Attachment #8662813 -
Flags: review?(roc)
Assignee | ||
Comment 2•9 years ago
|
||
Bug 1182426 - Part 2. Don't try to encode new frames of a size other than the initial in VP8TrackEncoder. r?roc
Attachment #8662814 -
Flags: review?(roc)
Assignee | ||
Comment 3•9 years ago
|
||
Bug 1182426 - Part 3. Flatten YUV formats conversion code in VP8TrackEncoder. r?roc
Attachment #8662815 -
Flags: review?(roc)
Assignee | ||
Comment 4•9 years ago
|
||
Bug 1182426 - Part 4. Convert CairoImages in VP8TrackEncoder. r?roc
Attachment #8662816 -
Flags: review?(roc)
Assignee | ||
Comment 5•9 years ago
|
||
Bug 1182426 - Part 5. Add some asserts to VP8TrackEncoder for sanity. r?roc
Attachment #8662817 -
Flags: review?(roc)
Assignee | ||
Comment 6•9 years ago
|
||
Bug 1182426 - Part 6. Test that changing video resolution of a recorded stream throws an error. r?roc
Attachment #8662818 -
Flags: review?(roc)
Assignee | ||
Comment 7•9 years ago
|
||
Bug 1182426 - Part 7. Test that we can record CanvasCaptureMediaStreams. r?roc
Attachment #8662819 -
Flags: review?(roc)
Assignee | ||
Comment 8•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → pehrsons
Assignee | ||
Comment 9•9 years ago
|
||
Bug 1182426 - Part 8. Set PlanarYCbCrImage's size in VP8TrackEncoder GTest. r?roc
Attachment #8662847 -
Flags: review?(roc)
Assignee | ||
Comment 10•9 years ago
|
||
Try push for the fixed gtest: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9c6abe3f8f4c
Status: NEW → ASSIGNED
Comment on attachment 8662813 [details]
MozReview Request: Bug 1182426 - Sort includes in VP8TrackEncoder.cpp alphabetically. r=roc
https://reviewboard.mozilla.org/r/19657/#review17687
Attachment #8662813 -
Flags: review?(roc) → review+
Comment on attachment 8662814 [details]
MozReview Request: Bug 1182426 - Don't try to encode new frames of a size other than the initial in VP8TrackEncoder. r=roc
https://reviewboard.mozilla.org/r/19659/#review17691
::: dom/media/encoder/VP8TrackEncoder.cpp:266
(Diff revision 1)
> + }
The MSG creates a 1x1 black pixel image to replace the video for cross-origin streams (see SetImageToBlackPixel). I guess that will get discarded here, which isn't great. How hard would it be to scale the image to the initial size here?
Attachment #8662814 -
Flags: review?(roc)
Assignee | ||
Comment 13•9 years ago
|
||
https://reviewboard.mozilla.org/r/19659/#review17691
> The MSG creates a 1x1 black pixel image to replace the video for cross-origin streams (see SetImageToBlackPixel). I guess that will get discarded here, which isn't great. How hard would it be to scale the image to the initial size here?
Hmm, I only see `SetImageToBlackPixel` used in `PlayVideo`, and isn't that only for sending video frames to their ImageContainer sink (i.e., the compositor)?
There is a check for `GetForceBlack()` before this size check that should kick in for cross-origin streams, though we should really have a mochitest for this if we don't already. I'll follow up on that.
I'd like to avoid scaling since there are so many unknowns that are not covered by the spec. E.g., do we always scale? only scale down? Crop or letterbox? Etc.
This is also closer to existing behavior. Before we'd encode garbage while we now raise the error event and stop the encoding. Should we want to scale we should file a new bug and fix it there.
(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #13)
> https://reviewboard.mozilla.org/r/19659/#review17691
> > The MSG creates a 1x1 black pixel image to replace the video for cross-origin streams (see SetImageToBlackPixel). I guess that will get discarded here, which isn't great. How hard would it be to scale the image to the initial size here?
>
> Hmm, I only see `SetImageToBlackPixel` used in `PlayVideo`, and isn't that
> only for sending video frames to their ImageContainer sink (i.e., the
> compositor)?
You're right. Sorry.
> I'd like to avoid scaling since there are so many unknowns that are not
> covered by the spec. E.g., do we always scale? only scale down? Crop or
> letterbox? Etc.
> This is also closer to existing behavior. Before we'd encode garbage while
> we now raise the error event and stop the encoding. Should we want to scale
> we should file a new bug and fix it there.
Rather than scaling, we should encode the dynamic resolution change. I'm assured this is possible in WebM and fragmented MP4.
(And yes, meat for another bug.)
Comment on attachment 8662814 [details]
MozReview Request: Bug 1182426 - Don't try to encode new frames of a size other than the initial in VP8TrackEncoder. r=roc
https://reviewboard.mozilla.org/r/19659/#review17779
Attachment #8662814 -
Flags: review+
Comment on attachment 8662815 [details]
MozReview Request: Bug 1182426 - Flatten YUV formats conversion code in VP8TrackEncoder. r=roc
https://reviewboard.mozilla.org/r/19661/#review17781
::: dom/media/encoder/VP8TrackEncoder.cpp:309
(Diff revision 1)
> + // Cast away constness b/c some of the accessors are non-const
Just fix the accessors!
Attachment #8662815 -
Flags: review?(roc) → review+
Comment on attachment 8662816 [details]
MozReview Request: Bug 1182426 - Convert non-PlanarYCbCRImages in VP8TrackEncoder. r=roc
https://reviewboard.mozilla.org/r/19663/#review17783
Attachment #8662816 -
Flags: review?(roc) → review+
Attachment #8662817 -
Flags: review?(roc) → review+
Comment on attachment 8662817 [details]
MozReview Request: Bug 1182426 - Add some asserts to VP8TrackEncoder for sanity. r=roc
https://reviewboard.mozilla.org/r/19665/#review17785
::: dom/media/encoder/VP8TrackEncoder.cpp:362
(Diff revision 1)
> + MOZ_ASSERT(false);
I think these asserts should be non-fatal (NS_ASSERTION). Unsupported format is an easily tolerated error and there's no point in cutting off a test suite run just because we hit this.
Attachment #8662818 -
Flags: review?(roc) → review+
Comment on attachment 8662818 [details]
MozReview Request: Bug 1182426 - Test that changing video resolution of a recorded stream throws an error. r=roc
https://reviewboard.mozilla.org/r/19667/#review17787
Comment on attachment 8662819 [details]
MozReview Request: Bug 1182426 - Test that we can record CanvasCaptureMediaStreams. r=roc
https://reviewboard.mozilla.org/r/19669/#review17789
Attachment #8662819 -
Flags: review?(roc) → review+
Comment on attachment 8662847 [details]
MozReview Request: Bug 1182426 - Set PlanarYCbCrImage's size in VP8TrackEncoder GTest. r=roc
https://reviewboard.mozilla.org/r/19671/#review17791
Attachment #8662847 -
Flags: review?(roc) → review+
Attachment #8662816 -
Flags: review+
Comment on attachment 8662816 [details]
MozReview Request: Bug 1182426 - Convert non-PlanarYCbCRImages in VP8TrackEncoder. r=roc
https://reviewboard.mozilla.org/r/19663/#review17793
You should change the commit message to say just "non-PlanarYCbCrImages" instead of CairoImage. This path handles all image types that implement GetAsSourceSurface, which should be all of them --- currently we have a few missing, but they should be fixed.
Assignee | ||
Comment 24•9 years ago
|
||
https://reviewboard.mozilla.org/r/19661/#review17781
> Just fix the accessors!
Tried just now but failed on the caching of the surface that we do in `GetAsSourceSurface()`.
Not sure why these were cast to const in the first place though because the base Image is non-const. I'll just remove the const here.
Assignee | ||
Comment 25•9 years ago
|
||
https://reviewboard.mozilla.org/r/19665/#review17785
> I think these asserts should be non-fatal (NS_ASSERTION). Unsupported format is an easily tolerated error and there's no point in cutting off a test suite run just because we hit this.
Done.
Assignee | ||
Updated•9 years ago
|
Assignee | ||
Updated•9 years ago
|
Attachment #8662813 -
Attachment description: MozReview Request: Bug 1182426 - Part 1. Sort includes in VP8TrackEncoder.cpp alphabetically. r?roc → MozReview Request: Bug 1182426 - Sort includes in VP8TrackEncoder.cpp alphabetically. r=roc
Assignee | ||
Comment 26•9 years ago
|
||
Comment on attachment 8662813 [details]
MozReview Request: Bug 1182426 - Sort includes in VP8TrackEncoder.cpp alphabetically. r=roc
Bug 1182426 - Sort includes in VP8TrackEncoder.cpp alphabetically. r=roc
Assignee | ||
Comment 27•9 years ago
|
||
Comment on attachment 8662814 [details]
MozReview Request: Bug 1182426 - Don't try to encode new frames of a size other than the initial in VP8TrackEncoder. r=roc
Bug 1182426 - Don't try to encode new frames of a size other than the initial in VP8TrackEncoder. r=roc
Attachment #8662814 -
Attachment description: MozReview Request: Bug 1182426 - Part 2. Don't try to encode new frames of a size other than the initial in VP8TrackEncoder. r?roc → MozReview Request: Bug 1182426 - Don't try to encode new frames of a size other than the initial in VP8TrackEncoder. r=roc
Assignee | ||
Updated•9 years ago
|
Attachment #8662815 -
Attachment description: MozReview Request: Bug 1182426 - Part 3. Flatten YUV formats conversion code in VP8TrackEncoder. r?roc → MozReview Request: Bug 1182426 - Flatten YUV formats conversion code in VP8TrackEncoder. r=roc
Assignee | ||
Comment 28•9 years ago
|
||
Comment on attachment 8662815 [details]
MozReview Request: Bug 1182426 - Flatten YUV formats conversion code in VP8TrackEncoder. r=roc
Bug 1182426 - Flatten YUV formats conversion code in VP8TrackEncoder. r=roc
Assignee | ||
Comment 29•9 years ago
|
||
Comment on attachment 8662816 [details]
MozReview Request: Bug 1182426 - Convert non-PlanarYCbCRImages in VP8TrackEncoder. r=roc
Bug 1182426 - Convert non-PlanarYCbCRImages in VP8TrackEncoder. r=roc
Attachment #8662816 -
Attachment description: MozReview Request: Bug 1182426 - Part 4. Convert CairoImages in VP8TrackEncoder. r?roc → MozReview Request: Bug 1182426 - Convert non-PlanarYCbCRImages in VP8TrackEncoder. r=roc
Assignee | ||
Updated•9 years ago
|
Attachment #8662817 -
Attachment description: MozReview Request: Bug 1182426 - Part 5. Add some asserts to VP8TrackEncoder for sanity. r?roc → MozReview Request: Bug 1182426 - Add some asserts to VP8TrackEncoder for sanity. r=roc
Assignee | ||
Comment 30•9 years ago
|
||
Comment on attachment 8662817 [details]
MozReview Request: Bug 1182426 - Add some asserts to VP8TrackEncoder for sanity. r=roc
Bug 1182426 - Add some asserts to VP8TrackEncoder for sanity. r=roc
Assignee | ||
Comment 31•9 years ago
|
||
Comment on attachment 8662818 [details]
MozReview Request: Bug 1182426 - Test that changing video resolution of a recorded stream throws an error. r=roc
Bug 1182426 - Test that changing video resolution of a recorded stream throws an error. r=roc
Attachment #8662818 -
Attachment description: MozReview Request: Bug 1182426 - Part 6. Test that changing video resolution of a recorded stream throws an error. r?roc → MozReview Request: Bug 1182426 - Test that changing video resolution of a recorded stream throws an error. r=roc
Assignee | ||
Comment 32•9 years ago
|
||
Comment on attachment 8662819 [details]
MozReview Request: Bug 1182426 - Test that we can record CanvasCaptureMediaStreams. r=roc
Bug 1182426 - Test that we can record CanvasCaptureMediaStreams. r=roc
Attachment #8662819 -
Attachment description: MozReview Request: Bug 1182426 - Part 7. Test that we can record CanvasCaptureMediaStreams. r?roc → MozReview Request: Bug 1182426 - Test that we can record CanvasCaptureMediaStreams. r=roc
Assignee | ||
Comment 33•9 years ago
|
||
Comment on attachment 8662847 [details]
MozReview Request: Bug 1182426 - Set PlanarYCbCrImage's size in VP8TrackEncoder GTest. r=roc
Bug 1182426 - Set PlanarYCbCrImage's size in VP8TrackEncoder GTest. r=roc
Attachment #8662847 -
Attachment description: MozReview Request: Bug 1182426 - Part 8. Set PlanarYCbCrImage's size in VP8TrackEncoder GTest. r?roc → MozReview Request: Bug 1182426 - Set PlanarYCbCrImage's size in VP8TrackEncoder GTest. r=roc
Assignee | ||
Comment 34•9 years ago
|
||
Comment on attachment 8662816 [details]
MozReview Request: Bug 1182426 - Convert non-PlanarYCbCRImages in VP8TrackEncoder. r=roc
Carries r=roc per comment 18.
Patches have been fixed per the comments, rebased, and the new tests have been disabled on Android and B2G with a ref to bug 1182426.
Attachment #8662816 -
Flags: review+
Assignee | ||
Comment 35•9 years ago
|
||
Assignee | ||
Comment 36•9 years ago
|
||
Ok, from that try push the timeouts look like my fault. Will take a look.
Assignee | ||
Comment 37•9 years ago
|
||
Comment on attachment 8662813 [details]
MozReview Request: Bug 1182426 - Sort includes in VP8TrackEncoder.cpp alphabetically. r=roc
Bug 1182426 - Sort includes in VP8TrackEncoder.cpp alphabetically. r=roc
Assignee | ||
Comment 38•9 years ago
|
||
Comment on attachment 8662814 [details]
MozReview Request: Bug 1182426 - Don't try to encode new frames of a size other than the initial in VP8TrackEncoder. r=roc
Bug 1182426 - Don't try to encode new frames of a size other than the initial in VP8TrackEncoder. r=roc
Assignee | ||
Comment 39•9 years ago
|
||
Comment on attachment 8662815 [details]
MozReview Request: Bug 1182426 - Flatten YUV formats conversion code in VP8TrackEncoder. r=roc
Bug 1182426 - Flatten YUV formats conversion code in VP8TrackEncoder. r=roc
Assignee | ||
Comment 40•9 years ago
|
||
Comment on attachment 8662816 [details]
MozReview Request: Bug 1182426 - Convert non-PlanarYCbCRImages in VP8TrackEncoder. r=roc
Bug 1182426 - Convert non-PlanarYCbCRImages in VP8TrackEncoder. r=roc
Attachment #8662816 -
Flags: review+
Assignee | ||
Comment 41•9 years ago
|
||
Comment on attachment 8662817 [details]
MozReview Request: Bug 1182426 - Add some asserts to VP8TrackEncoder for sanity. r=roc
Bug 1182426 - Add some asserts to VP8TrackEncoder for sanity. r=roc
Assignee | ||
Comment 42•9 years ago
|
||
Comment on attachment 8662818 [details]
MozReview Request: Bug 1182426 - Test that changing video resolution of a recorded stream throws an error. r=roc
Bug 1182426 - Test that changing video resolution of a recorded stream throws an error. r=roc
Assignee | ||
Comment 43•9 years ago
|
||
Comment on attachment 8662819 [details]
MozReview Request: Bug 1182426 - Test that we can record CanvasCaptureMediaStreams. r=roc
Bug 1182426 - Test that we can record CanvasCaptureMediaStreams. r=roc
Assignee | ||
Comment 44•9 years ago
|
||
Comment on attachment 8662847 [details]
MozReview Request: Bug 1182426 - Set PlanarYCbCrImage's size in VP8TrackEncoder GTest. r=roc
Bug 1182426 - Set PlanarYCbCrImage's size in VP8TrackEncoder GTest. r=roc
Assignee | ||
Comment 45•9 years ago
|
||
Comment on attachment 8662816 [details]
MozReview Request: Bug 1182426 - Convert non-PlanarYCbCRImages in VP8TrackEncoder. r=roc
Carries r=roc.
Attachment #8662816 -
Flags: review+
Assignee | ||
Comment 46•9 years ago
|
||
I had taken away the canvas.capturestream.enabled pref in the tests but forgot to call startTest() manually.
New try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=23b1f15df36f
Comment 47•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/2867a0d9bc49
https://hg.mozilla.org/integration/mozilla-inbound/rev/c32c569f342d
https://hg.mozilla.org/integration/mozilla-inbound/rev/0b12e6d710d0
https://hg.mozilla.org/integration/mozilla-inbound/rev/79a13fd1abf7
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e8b725ab2aa
https://hg.mozilla.org/integration/mozilla-inbound/rev/c519531a1e4c
https://hg.mozilla.org/integration/mozilla-inbound/rev/f51c9035e472
https://hg.mozilla.org/integration/mozilla-inbound/rev/c1795d9a7b42
https://hg.mozilla.org/mozilla-central/rev/2867a0d9bc49
https://hg.mozilla.org/mozilla-central/rev/c32c569f342d
https://hg.mozilla.org/mozilla-central/rev/0b12e6d710d0
https://hg.mozilla.org/mozilla-central/rev/79a13fd1abf7
https://hg.mozilla.org/mozilla-central/rev/6e8b725ab2aa
https://hg.mozilla.org/mozilla-central/rev/c519531a1e4c
https://hg.mozilla.org/mozilla-central/rev/f51c9035e472
https://hg.mozilla.org/mozilla-central/rev/c1795d9a7b42
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox44:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in
before you can comment on or make changes to this bug.
Description
•