1182426 - Make MediaRecorder support all image formats

Status: RESOLVED FIXED

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

Description

[Andreas Pehrson [:pehrsons]]

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

Comment 1

[Andreas Pehrson [:pehrsons]]

Attachment: MozReview Request: Bug 1182426 - Sort includes in VP8TrackEncoder.cpp alphabetically. r=roc

Bug 1182426 - Part 1. Sort includes in VP8TrackEncoder.cpp alphabetically. r?roc

Comment 2

[Andreas Pehrson [:pehrsons]]

Attachment: MozReview Request: Bug 1182426 - Don't try to encode new frames of a size other than the initial in VP8TrackEncoder. r=roc

Bug 1182426 - Part 2. Don't try to encode new frames of a size other than the initial in VP8TrackEncoder. r?roc

Comment 3

[Andreas Pehrson [:pehrsons]]

Attachment: MozReview Request: Bug 1182426 - Flatten YUV formats conversion code in VP8TrackEncoder. r=roc

Bug 1182426 - Part 3. Flatten YUV formats conversion code in VP8TrackEncoder. r?roc

Comment 4

[Andreas Pehrson [:pehrsons]]

Attachment: MozReview Request: Bug 1182426 - Convert non-PlanarYCbCRImages in VP8TrackEncoder. r=roc

Bug 1182426 - Part 4. Convert CairoImages in VP8TrackEncoder. r?roc

Comment 5

[Andreas Pehrson [:pehrsons]]

Attachment: MozReview Request: Bug 1182426 - Add some asserts to VP8TrackEncoder for sanity. r=roc

Bug 1182426 - Part 5. Add some asserts to VP8TrackEncoder for sanity. r?roc

Comment 6

[Andreas Pehrson [:pehrsons]]

Attachment: MozReview Request: Bug 1182426 - Test that changing video resolution of a recorded stream throws an error. r=roc

Bug 1182426 - Part 6. Test that changing video resolution of a recorded stream throws an error. r?roc

Comment 7

[Andreas Pehrson [:pehrsons]]

Attachment: MozReview Request: Bug 1182426 - Test that we can record CanvasCaptureMediaStreams. r=roc

Bug 1182426 - Part 7. Test that we can record CanvasCaptureMediaStreams. r?roc

Comment 8

[Andreas Pehrson [:pehrsons]]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=387ea55adc57

Comment 9

[Andreas Pehrson [:pehrsons]]

Attachment: MozReview Request: Bug 1182426 - Set PlanarYCbCrImage's size in VP8TrackEncoder GTest. r=roc

Bug 1182426 - Part 8. Set PlanarYCbCrImage's size in VP8TrackEncoder GTest. r?roc

Comment 10

[Andreas Pehrson [:pehrsons]]

Try push for the fixed gtest: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9c6abe3f8f4c

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8662813 [details]
MozReview Request: Bug 1182426 - Sort includes in VP8TrackEncoder.cpp alphabetically. r=roc

https://reviewboard.mozilla.org/r/19657/#review17687

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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?

Comment 13

[Andreas Pehrson [:pehrsons]]

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.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(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.

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(And yes, meat for another bug.)

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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!

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8662816 [details]
MozReview Request: Bug 1182426 - Convert non-PlanarYCbCRImages in VP8TrackEncoder. r=roc

https://reviewboard.mozilla.org/r/19663/#review17783

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8662819 [details]
MozReview Request: Bug 1182426 - Test that we can record CanvasCaptureMediaStreams. r=roc

https://reviewboard.mozilla.org/r/19669/#review17789

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 24

[Andreas Pehrson [:pehrsons]]

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.

Comment 25

[Andreas Pehrson [:pehrsons]]

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.

Comment 26

[Andreas Pehrson [:pehrsons]]

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 27

[Andreas Pehrson [:pehrsons]]

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 28

[Andreas Pehrson [:pehrsons]]

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 29

[Andreas Pehrson [:pehrsons]]

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

Comment 30

[Andreas Pehrson [:pehrsons]]

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 31

[Andreas Pehrson [:pehrsons]]

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 32

[Andreas Pehrson [:pehrsons]]

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 33

[Andreas Pehrson [:pehrsons]]

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 34

[Andreas Pehrson [:pehrsons]]

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.

Comment 35

[Andreas Pehrson [:pehrsons]]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=37a8be73c4ed

Comment 36

[Andreas Pehrson [:pehrsons]]

Ok, from that try push the timeouts look like my fault. Will take a look.

Comment 37

[Andreas Pehrson [:pehrsons]]

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 38

[Andreas Pehrson [:pehrsons]]

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 39

[Andreas Pehrson [:pehrsons]]

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 40

[Andreas Pehrson [:pehrsons]]

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

Comment 41

[Andreas Pehrson [:pehrsons]]

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 42

[Andreas Pehrson [:pehrsons]]

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 43

[Andreas Pehrson [:pehrsons]]

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 44

[Andreas Pehrson [:pehrsons]]

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 45

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8662816 [details]
MozReview Request: Bug 1182426 - Convert non-PlanarYCbCRImages in VP8TrackEncoder. r=roc

Carries r=roc.

Comment 46

[Andreas Pehrson [:pehrsons]]

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

[Pulsebot]

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

Comment 48

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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