Make MediaRecorder support all image formats

RESOLVED FIXED in Firefox 44

Status

()

Core
Audio/Video: Recording
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: pehrsons, Assigned: pehrsons)

Tracking

unspecified
mozilla44
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox44 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(8 attachments)

40 bytes, text/x-review-board-request
roc
: review+
Details | Review
40 bytes, text/x-review-board-request
roc
: review+
Details | Review
40 bytes, text/x-review-board-request
roc
: review+
Details | Review
40 bytes, text/x-review-board-request
pehrsons
: review+
Details | Review
40 bytes, text/x-review-board-request
roc
: review+
Details | Review
40 bytes, text/x-review-board-request
roc
: review+
Details | Review
40 bytes, text/x-review-board-request
roc
: review+
Details | Review
40 bytes, text/x-review-board-request
roc
: review+
Details | Review
(Assignee)

Description

3 years ago
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)

Updated

3 years ago
Blocks: 1177276
(Assignee)

Comment 1

3 years ago
Created attachment 8662813 [details]
MozReview Request: Bug 1182426 - Sort includes in VP8TrackEncoder.cpp alphabetically. r=roc

Bug 1182426 - Part 1. Sort includes in VP8TrackEncoder.cpp alphabetically. r?roc
Attachment #8662813 - Flags: review?(roc)
(Assignee)

Comment 2

3 years ago
Created 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 - 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

3 years ago
Created attachment 8662815 [details]
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
Attachment #8662815 - Flags: review?(roc)
(Assignee)

Comment 4

3 years ago
Created attachment 8662816 [details]
MozReview Request: Bug 1182426 - Convert non-PlanarYCbCRImages in VP8TrackEncoder. r=roc

Bug 1182426 - Part 4. Convert CairoImages in VP8TrackEncoder. r?roc
Attachment #8662816 - Flags: review?(roc)
(Assignee)

Comment 5

3 years ago
Created attachment 8662817 [details]
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
Attachment #8662817 - Flags: review?(roc)
(Assignee)

Comment 6

3 years ago
Created attachment 8662818 [details]
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
Attachment #8662818 - Flags: review?(roc)
(Assignee)

Comment 7

3 years ago
Created attachment 8662819 [details]
MozReview Request: Bug 1182426 - Test that we can record CanvasCaptureMediaStreams. r=roc

Bug 1182426 - Part 7. Test that we can record CanvasCaptureMediaStreams. r?roc
Attachment #8662819 - Flags: review?(roc)
(Assignee)

Updated

3 years ago
Assignee: nobody → pehrsons
(Assignee)

Comment 9

3 years ago
Created attachment 8662847 [details]
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
Attachment #8662847 - Flags: review?(roc)
(Assignee)

Comment 10

3 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

3 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+
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.
(Assignee)

Comment 24

3 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

3 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

3 years ago
See Also: → bug 1210286
(Assignee)

Updated

3 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

3 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

3 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

3 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

3 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

3 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

3 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

3 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

3 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

3 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

3 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

3 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 36

3 years ago
Ok, from that try push the timeouts look like my fault. Will take a look.
(Assignee)

Comment 37

3 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

3 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

3 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

3 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

3 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

3 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

3 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

3 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

3 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

3 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

Updated

3 years ago
Duplicate of this bug: 1224111

Updated

2 years ago
Depends on: 1272877
You need to log in before you can comment on or make changes to this bug.