Closed
Bug 1421187
Opened 7 years ago
Closed 7 years ago
Software images path on MacOS produces incorrect colours
Categories
(Core :: Audio/Video: Playback, defect, P3)
Core
Audio/Video: Playback
Tracking
()
RESOLVED
FIXED
mozilla59
People
(Reporter: cpearce, Assigned: jya)
References
Details
(Keywords: regression)
Attachments
(5 files)
(deleted),
image/png
|
Details | |
(deleted),
text/x-review-board-request
|
mattwoodrow
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
mattwoodrow
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
mattwoodrow
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
mattwoodrow
:
review+
|
Details |
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
Assignee | ||
Comment 1•7 years ago
|
||
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 | ||
Updated•7 years ago
|
Assignee: nobody → jyavenard
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 6•7 years ago
|
||
mozreview-review |
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 7•7 years ago
|
||
mozreview-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 8•7 years ago
|
||
mozreview-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 9•7 years ago
|
||
mozreview-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+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 14•7 years ago
|
||
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
Comment 15•7 years ago
|
||
bugherder |
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
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox59:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Updated•7 years ago
|
status-firefox57:
--- → wontfix
status-firefox58:
--- → affected
status-firefox-esr52:
--- → wontfix
Updated•7 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•