Canvas regression: Blank canvas with putImageData
Reported by
acmesqua...@gmail.com,
Jan 23 2017
|
|||||||||
Issue description
UserAgent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:51.0) Gecko/20100101 Firefox/51.0
Steps to reproduce the problem:
putImageData to a canvas context using {alpha:false}
What is the expected behavior?
ImageData is drawn to canvas (Ignoring transparency)
What went wrong?
Canvas is black
Did this work before? Yes 56.0.2924.67 beta (64-bit)
Does this work in other browsers? Yes
Chrome version: 57.0.2986.0 dev (64-bit) Channel: dev
OS Version:
Flash Version:
,
Jan 23 2017
Able to reproduce the issue on windows-7, Mac 10.12.2 and Linux Ubuntu-14.04 using chrome canary 58.0.2989.0 This is regression issue broken in M57. Please find the bisect information as below Narrow Bisect:: =============== Good :57.0.2985.0 -- (build revision 444244) Bad:: 57.0.2986.0 -- (build revision 444600) ChangeLog: ================ https://chromium.googlesource.com/chromium/src/+log/576c8fd139e3308796d3a0f1a0b859cbc3c0d5ea..24a43afbe8773249e2ad7552bba40a07da528539 Possible suspect ================== https://skia.googlesource.com/skia.git/+/92ce5946855aa8d55bb4a0dd0a47d58746d67d0a Brian Salomon @ could you please look into this issue if it is related to your change,else please help us in finding the appropriate owner for this issue. Thanks,
,
Jan 23 2017
It seems like this is most likely: https://skia-review.googlesource.com/c/7121/ (SkImageInfoValidConversion (part 2)) but it could also be: https://skia-review.googlesource.com/c/7172/ (Move read/write-Pixels up to GrSurfaceContext)
,
Jan 23 2017
This looks to have been caused my change. I'll look into it.
,
Jan 23 2017
I'm planning to land and cherry-pick an immediate fix: https://skia-review.googlesource.com/c/7374/ But I think it'd be good to discuss how we expect to behave when writing/drawing non-opaque pixels (src) to an opaque surface (dst). First, what is the expected behavior from the canvas API? Chrome seems to force the src alpha values to one (which is funny because I don't think Skia does this) - see that putImageData-test draws a red square in Chrome. Firefox seems to premultiply or blend with src alphas - see that putImageData-test draws a black square (and the original test is a darker gray). Second, how should Skia behave? (1) I believe that the old behavior was "undefined" - but most of the time that meant treating the src alpha as 0xFF even if it wasn't. Opaque is an optimization hint "guaranteeing" that pixel alphas are 0xFF. (2) I've tried to just make non-opaque->opaque illegal in readPixels()/writePixels(), which is at least well-defined... But inconsistent with drawing behavior. (3) Another option is to actually force all the alphas to 0xFF - this is more work for Skia and a performance cost, but seems to match how Chrome behaves now. Adding junov@, reed@, and fmalita@ for thoughts.
,
Jan 23 2017
The following revision refers to this bug: https://skia.googlesource.com/skia.git/+/bfe8dca7dfdd9cafbccba0a637f2fcd58c7a54fb commit bfe8dca7dfdd9cafbccba0a637f2fcd58c7a54fb Author: Matt Sarett <msarett@google.com> Date: Mon Jan 23 14:41:08 2017 Allow conversion from non-opaque to opaque BUG:683787 Change-Id: I1b78cc8d1b5d3917a2a952da036b93022e99e053 Reviewed-on: https://skia-review.googlesource.com/7374 Reviewed-by: Robert Phillips <robertphillips@google.com> Commit-Queue: Matt Sarett <msarett@google.com> [modify] https://crrev.com/bfe8dca7dfdd9cafbccba0a637f2fcd58c7a54fb/src/core/SkImageInfoPriv.h
,
Jan 23 2017
,
Jan 24 2017
Your change meets the bar and is auto-approved for M57. Please go ahead and merge the CL to branch 2987 manually. Please contact milestone owner if you have questions. Owners: amineer@(clank), cmasso@(bling), ketakid@(cros), govind@(desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jan 24 2017
Please merge your change to M57 branch 2987 ASAP so we can pick it up for this week dev release. Thank you.
,
Jan 24 2017
The following revision refers to this bug: https://skia.googlesource.com/skia.git/+/e3917fd5ca950222facf027a73d95c8339bced54 commit e3917fd5ca950222facf027a73d95c8339bced54 Author: Matt Sarett <msarett@google.com> Date: Mon Jan 23 14:41:08 2017 Allow conversion from non-opaque to opaque [chrome/m57] BUG:683787 Change-Id: I1b78cc8d1b5d3917a2a952da036b93022e99e053 Reviewed-on: https://skia-review.googlesource.com/7374 Reviewed-by: Robert Phillips <robertphillips@google.com> Commit-Queue: Matt Sarett <msarett@google.com> (cherry picked from commit bfe8dca7dfdd9cafbccba0a637f2fcd58c7a54fb) Reviewed-on: https://skia-review.googlesource.com/7402 [modify] https://crrev.com/e3917fd5ca950222facf027a73d95c8339bced54/src/core/SkImageInfoPriv.h
,
Jan 24 2017
,
Jan 24 2017
Per comment #11 this is already merged to M57. Hence removing "Merge-Approved-57" label.
,
Feb 6 2017
The following revision refers to this bug: https://skia.googlesource.com/skia/+/bfe8dca7dfdd9cafbccba0a637f2fcd58c7a54fb commit bfe8dca7dfdd9cafbccba0a637f2fcd58c7a54fb Author: Matt Sarett <msarett@google.com> Date: Mon Jan 23 15:47:42 2017 Allow conversion from non-opaque to opaque BUG:683787 Change-Id: I1b78cc8d1b5d3917a2a952da036b93022e99e053 Reviewed-on: https://skia-review.googlesource.com/7374 Reviewed-by: Robert Phillips <robertphillips@google.com> Commit-Queue: Matt Sarett <msarett@google.com> [modify] https://crrev.com/bfe8dca7dfdd9cafbccba0a637f2fcd58c7a54fb/src/core/SkImageInfoPriv.h
,
Feb 6 2017
The following revision refers to this bug: https://skia.googlesource.com/skia/+/a8f413927951e1b6e6c791526da3c92bc45639e2 commit a8f413927951e1b6e6c791526da3c92bc45639e2 Author: Matt Sarett <msarett@google.com> Date: Mon Feb 06 19:03:54 2017 Revert "Allow conversion from non-opaque to opaque" This reverts commit bfe8dca7dfdd9cafbccba0a637f2fcd58c7a54fb. Reason for revert: I believe I've fixed all the call sites, so this is now ok. Original change's description: > Allow conversion from non-opaque to opaque > > BUG:683787 > > Change-Id: I1b78cc8d1b5d3917a2a952da036b93022e99e053 > Reviewed-on: https://skia-review.googlesource.com/7374 > Reviewed-by: Robert Phillips <robertphillips@google.com> > Commit-Queue: Matt Sarett <msarett@google.com> > TBR=msarett@google.com,robertphillips@google.com,reviews@skia.org # Not skipping CQ checks because original CL landed > 1 day ago. Change-Id: I2369c9b81321ca83a7063bb85e66ddbc03914d2b Reviewed-on: https://skia-review.googlesource.com/8073 Commit-Queue: Matt Sarett <msarett@google.com> Reviewed-by: Matt Sarett <msarett@google.com> [modify] https://crrev.com/a8f413927951e1b6e6c791526da3c92bc45639e2/src/core/SkImageInfoPriv.h
,
Feb 6 2017
The following revision refers to this bug: https://skia.googlesource.com/skia/+/a8f413927951e1b6e6c791526da3c92bc45639e2 commit a8f413927951e1b6e6c791526da3c92bc45639e2 Author: Matt Sarett <msarett@google.com> Date: Mon Feb 06 19:03:54 2017 Revert "Allow conversion from non-opaque to opaque" This reverts commit bfe8dca7dfdd9cafbccba0a637f2fcd58c7a54fb. Reason for revert: I believe I've fixed all the call sites, so this is now ok. Original change's description: > Allow conversion from non-opaque to opaque > > BUG:683787 > > Change-Id: I1b78cc8d1b5d3917a2a952da036b93022e99e053 > Reviewed-on: https://skia-review.googlesource.com/7374 > Reviewed-by: Robert Phillips <robertphillips@google.com> > Commit-Queue: Matt Sarett <msarett@google.com> > TBR=msarett@google.com,robertphillips@google.com,reviews@skia.org # Not skipping CQ checks because original CL landed > 1 day ago. Change-Id: I2369c9b81321ca83a7063bb85e66ddbc03914d2b Reviewed-on: https://skia-review.googlesource.com/8073 Commit-Queue: Matt Sarett <msarett@google.com> Reviewed-by: Matt Sarett <msarett@google.com> [modify] https://crrev.com/a8f413927951e1b6e6c791526da3c92bc45639e2/src/core/SkImageInfoPriv.h |
|||||||||
►
Sign in to add a comment |
|||||||||
Comment 1 by nyerramilli@chromium.org
, Jan 23 2017