1421187 - Software images path on MacOS produces incorrect colours

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, defect, P3)

Description

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Screenshot-2017-11-28 http pearce org nz.png

There's something wrong with our non-accelerated path on MacOS. If you set AppleVTDecoder::mUseSoftwareImages = false, you get incorrect colours. Note we don't ship this configuration in Firefox by default. See attachment for example.

We noticed this as we were using the software path in gecko-media. I r-'d a patch [1] that worked around the problem by removing taking into account the stride in the destination plane our copy of ImageContainer.cpp's CopyPlane(). So I'm guessing that we're not getting the stride info correct in the CPU memory case somehow.

[1] https://github.com/servo/gecko-media/pull/106/commits/4d7b918cae15641933dd3ec896d0825e0bde8d03

Comment 1

[Jean-Yves Avenard [:jya]]

The regression occurs with bug 1200595...

It doesn't take into account mSkip when copying the content, assuming that so long as the stride is equal, then we can just copy the data.

That is true, but then skip value in the destination surface should be set

Comment 2

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1421187 - P1. Don't use fast path copy when CbCr channel interleaved.

Review commit: https://reviewboard.mozilla.org/r/203542/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/203542/

Comment 3

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1421187 - P2. Properly set bytesPerPixel in MappedYCbCrTextureData.

Review commit: https://reviewboard.mozilla.org/r/203544/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/203544/

Comment 4

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1421187 - P3. Optimize pixels data copy and remove extra loop.

There's no need to perform the format test within the loop, so we can separate the different cases as needed.
Also copy the entire pixel data in one go, by using C types.

The skip value definition doesn't specify if it's in bytes, or in "pixels". We will assume the later. There are currently no decoders returning HDR content with a skip value different than zero anyway.

Review commit: https://reviewboard.mozilla.org/r/203546/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/203546/

Comment 5

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1421187 - P4. Make mac decoder output YUV420 format.

There's little advantage over using NV12 and using YUV420 allows for easier and faster frame copy. We assume of course that Apple's VideoToolbox have more optimized code than we do.

Review commit: https://reviewboard.mozilla.org/r/203548/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/203548/

Comment 6

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8932514 [details]
Bug 1421187 - P3. Optimize pixels data copy and remove extra loop.

https://reviewboard.mozilla.org/r/203546/#review209104

::: commit-message-6c373:6
(Diff revision 1)
> +Bug 1421187 - P3. Optimize pixels data copy and remove extra loop. r?mattwoodrow
> +
> +There's no need to perform the format test within the loop, so we can separate the different cases as needed.
> +Also copy the entire pixel data in one go, by using C types.
> +
> +The skip value definition doesn't specify if it's in bytes, or in "pixels". We will assume the later. There are currently no decoders returning HDR content with a skip value different than zero anyway.

Should we add an NS_WARNING for the case where we get HDR content and a non zero skip value?

That way, if we ever start using a decoder that hits it, we'll know where to look to make sure we're using the right units for skip.

Comment 7

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8932515 [details]
Bug 1421187 - P4. Make mac decoder output YUV420 format.

https://reviewboard.mozilla.org/r/203548/#review209106

Comment 8

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8932512 [details]
Bug 1421187 - P1. Don't use fast path copy when CbCr channel interleaved.

https://reviewboard.mozilla.org/r/203542/#review209108

Comment 9

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8932513 [details]
Bug 1421187 - P2. Properly set bytesPerPixel in MappedYCbCrTextureData.

https://reviewboard.mozilla.org/r/203544/#review209110

Comment 10

[Jean-Yves Avenard [:jya]]

Comment on attachment 8932512 [details]
Bug 1421187 - P1. Don't use fast path copy when CbCr channel interleaved.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/203542/diff/1-2/

Comment 11

[Jean-Yves Avenard [:jya]]

Comment on attachment 8932513 [details]
Bug 1421187 - P2. Properly set bytesPerPixel in MappedYCbCrTextureData.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/203544/diff/1-2/

Comment 12

[Jean-Yves Avenard [:jya]]

Comment on attachment 8932514 [details]
Bug 1421187 - P3. Optimize pixels data copy and remove extra loop.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/203546/diff/1-2/

Comment 13

[Jean-Yves Avenard [:jya]]

Comment on attachment 8932515 [details]
Bug 1421187 - P4. Make mac decoder output YUV420 format.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/203548/diff/1-2/

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c0c61b0ce2be
P1. Don't use fast path copy when CbCr channel interleaved. r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/4f71d73f7fd0
P2. Properly set bytesPerPixel in MappedYCbCrTextureData. r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/7b69819d8e0e
P3. Optimize pixels data copy and remove extra loop. r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/e9ac2319092d
P4. Make mac decoder output YUV420 format. r=mattwoodrow

Comment 15

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/c0c61b0ce2be
https://hg.mozilla.org/mozilla-central/rev/4f71d73f7fd0
https://hg.mozilla.org/mozilla-central/rev/7b69819d8e0e
https://hg.mozilla.org/mozilla-central/rev/e9ac2319092d