Closed Bug 1182426 Opened 5 years ago Closed 4 years ago

Make MediaRecorder support all image formats

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: pehrsons, Assigned: pehrsons)

References

Details

Attachments

(8 files)

40 bytes, text/x-review-board-request
roc
: review+
Details
40 bytes, text/x-review-board-request
roc
: review+
Details
40 bytes, text/x-review-board-request
roc
: review+
Details
40 bytes, text/x-review-board-request
pehrsons
: review+
Details
40 bytes, text/x-review-board-request
roc
: review+
Details
40 bytes, text/x-review-board-request
roc
: review+
Details
40 bytes, text/x-review-board-request
roc
: review+
Details
40 bytes, 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
Blocks: 1177276
Bug 1182426 - Part 1. Sort includes in VP8TrackEncoder.cpp alphabetically. r?roc
Attachment #8662813 - Flags: review?(roc)
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)
Bug 1182426 - Part 3. Flatten YUV formats conversion code in VP8TrackEncoder. r?roc
Attachment #8662815 - Flags: review?(roc)
Bug 1182426 - Part 4. Convert CairoImages in VP8TrackEncoder. r?roc
Attachment #8662816 - Flags: review?(roc)
Bug 1182426 - Part 5. Add some asserts to VP8TrackEncoder for sanity. r?roc
Attachment #8662817 - Flags: review?(roc)
Bug 1182426 - Part 6. Test that changing video resolution of a recorded stream throws an error. r?roc
Attachment #8662818 - Flags: review?(roc)
Bug 1182426 - Part 7. Test that we can record CanvasCaptureMediaStreams. r?roc
Attachment #8662819 - Flags: review?(roc)
Assignee: nobody → pehrsons
Bug 1182426 - Part 8. Set PlanarYCbCrImage's size in VP8TrackEncoder GTest. r?roc
Attachment #8662847 - Flags: review?(roc)
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)
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+
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.
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+
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.
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.
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.
See Also: → 1210286
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
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
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
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
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
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
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
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
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
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
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
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+
Ok, from that try push the timeouts look like my fault. Will take a look.
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
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
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
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+
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
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
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
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
Comment on attachment 8662816 [details]
MozReview Request: Bug 1182426 - Convert non-PlanarYCbCRImages in VP8TrackEncoder. r=roc

Carries r=roc.
Attachment #8662816 - Flags: review+
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
Duplicate of this bug: 1224111
Depends on: 1272877
You need to log in before you can comment on or make changes to this bug.