Closed Bug 1421187 Opened 2 years ago Closed 2 years ago

Software images path on MacOS produces incorrect colours

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox-esr52 --- wontfix
firefox57 --- wontfix
firefox58 --- wontfix
firefox59 --- fixed

People

(Reporter: cpearce, Assigned: jya)

References

Details

(Keywords: regression)

Attachments

(5 files)

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
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
Blocks: 1200595
Keywords: regression
Assignee: nobody → jyavenard
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.
Attachment #8932514 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8932515 [details]
Bug 1421187 - P4. Make mac decoder output YUV420 format.

https://reviewboard.mozilla.org/r/203548/#review209106
Attachment #8932515 - Flags: review?(matt.woodrow) → review+
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
Attachment #8932512 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8932513 [details]
Bug 1421187 - P2. Properly set bytesPerPixel in MappedYCbCrTextureData.

https://reviewboard.mozilla.org/r/203544/#review209110
Attachment #8932513 - Flags: review?(matt.woodrow) → review+
Pushed by jyavenard@mozilla.com:
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
You need to log in before you can comment on or make changes to this bug.