Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2332743003:
Disable png encodes from Alpha8, Float16 (Closed)
DescriptionDisable png encodes from Alpha8, Float16
These don't behave as we would want anyway. They just copy
to N32, and then encode.
BUG=skia:5616
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2332743003
Committed: https://skia.googlesource.com/skia/+/029819baa37f2706e6669f97badb20d309568d7c
Patch Set 1 #
Total comments: 2
Patch Set 2 : Removed unused bitmap copy #
Total comments: 1
Patch Set 3 : Add const #Patch Set 4 : Rebase #Messages
Total messages: 38 (28 generated)
Description was changed from ========== Disable png encodes to Alpha8, Float16 These don't behave as we would want anyway. They just copy to N32. BUG=skia: ========== to ========== Disable png encodes to Alpha8, Float16 These don't behave as we would want anyway. They just copy to N32. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2332743003 ==========
msarett@google.com changed reviewers: + reed@google.com
msarett@google.com changed reviewers: + scroggo@google.com
lgtm
> Disable png encodes to Alpha8, Float16 I think you mean "from" Alpha8, Float16? > These don't behave as we would want anyway. They just copy > to N32. What would we want? Are you sure no one is relying on the current behavior for Alpha8? https://codereview.chromium.org/2332743003/diff/1/src/images/SkPNGImageEncode... File src/images/SkPNGImageEncoder.cpp (right): https://codereview.chromium.org/2332743003/diff/1/src/images/SkPNGImageEncode... src/images/SkPNGImageEncoder.cpp:187: SkBitmap copy; It looks like "copy" is no longer used.
Description was changed from ========== Disable png encodes to Alpha8, Float16 These don't behave as we would want anyway. They just copy to N32. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2332743003 ========== to ========== Disable png encodes to Alpha8, Float16 These don't behave as we would want anyway. They just copy to N32. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2332743003 ==========
Description was changed from ========== Disable png encodes to Alpha8, Float16 These don't behave as we would want anyway. They just copy to N32. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2332743003 ========== to ========== Disable png encodes to Alpha8, Float16 These don't behave as we would want anyway. They just copy to N32. BUG=skia:5616 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2332743003 ==========
> > Disable png encodes to Alpha8, Float16 > I think you mean "from" Alpha8, Float16? Yes, fixed. > > These don't behave as we would want anyway. They just copy > > to N32. > What would we want? Are you sure no one is relying on the current behavior for > Alpha8? What we want is an Alpha8 encoder that can round-trip (ex: when we decode, we get Alpha8 back). Not sure how we're going to do this yet, but it's likely that we won't use PNG (no support for alpha only). I've referenced bug 5616, which I should have linked before. I'm not entirely sure that no one is using this... Was going to land and see what breaks (hopefully nothing). https://codereview.chromium.org/2332743003/diff/1/src/images/SkPNGImageEncode... File src/images/SkPNGImageEncoder.cpp (right): https://codereview.chromium.org/2332743003/diff/1/src/images/SkPNGImageEncode... src/images/SkPNGImageEncoder.cpp:187: SkBitmap copy; On 2016/09/12 17:23:29, scroggo wrote: > It looks like "copy" is no longer used. Removed.
Description was changed from ========== Disable png encodes to Alpha8, Float16 These don't behave as we would want anyway. They just copy to N32. BUG=skia:5616 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2332743003 ========== to ========== Disable png encodes from Alpha8, Float16 These don't behave as we would want anyway. They just copy to N32. BUG=skia:5616 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2332743003 ==========
scroggo@google.com changed reviewers: + djsollen@google.com, halcanary@google.com
On 2016/09/12 18:03:44, msarett wrote: > > > Disable png encodes to Alpha8, Float16 > > > I think you mean "from" Alpha8, Float16? > > Yes, fixed. > > > > These don't behave as we would want anyway. They just copy > > > to N32. "They just copy to N32 *and then encode*" (Maybe that's obvious.) > > > What would we want? Are you sure no one is relying on the current behavior for > > Alpha8? > > What we want is an Alpha8 encoder that can round-trip (ex: > when we decode, we get Alpha8 back). Not sure how we're > going to do this yet, but it's likely that we won't use > PNG (no support for alpha only). > > I've referenced bug 5616, which I should have linked > before. > > I'm not entirely sure that no one is using this... > Was going to land and see what breaks (hopefully nothing). Unfortunately, this is exposed to Android developers, via Bitmap.Compress. So I could imagine that an Android developer relies on it. That said, looking at https://b.corp.google.com/issues/23818651#comment12, it looks like we used to return false for Alpha_8. The existing code goes back to Hal's crrev.com/1506663002, which made it into nyc-dev (see https://googleplex-android.git.corp.google.com/platform/external/skia/+/nyc-d...) but not mnc-dev (see https://googleplex-android.git.corp.google.com/platform/external/skia/+/mnc-d...). So this would only break people who try to encode ALPHA_8 Bitmaps as PNGs in N, and they would already have to have code for prior to N (unless it's a new app that doesn't support older versions - I don't have statistics on that, but I'm guessing it's unlikely...). Regardless, presumably Hal depends on this behavior, so I'm guessing this will break his code. It ought to be a simple fix though - where Hal uses this, he needs to make the check and copy himself. Adding Hal for PDF concerns, and Derek for concerns about Android backwards compatibility. https://codereview.chromium.org/2332743003/diff/20001/src/images/SkPNGImageEn... File src/images/SkPNGImageEncoder.cpp (right): https://codereview.chromium.org/2332743003/diff/20001/src/images/SkPNGImageEn... src/images/SkPNGImageEncoder.cpp:187: SkColorType ct = bitmap.colorType(); nit: Could be const?
On 2016/09/12 18:23:00, scroggo wrote: > Adding Hal for PDF concerns No PDF concerns. I don't rely on this. I think I noticed this when I was testing something.
Description was changed from ========== Disable png encodes from Alpha8, Float16 These don't behave as we would want anyway. They just copy to N32. BUG=skia:5616 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2332743003 ========== to ========== Disable png encodes from Alpha8, Float16 These don't behave as we would want anyway. They just copy to N32, and then encode. BUG=skia:5616 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2332743003 ==========
Patchset #4 (id:60001) has been deleted
On 2016/09/12 18:28:59, Hal Canary wrote: > On 2016/09/12 18:23:00, scroggo wrote: > > Adding Hal for PDF concerns > > No PDF concerns. I don't rely on this. I think I noticed this when I was > testing something. Based on our conversation in person, I think it's ok to go ahead and land this. We only shipped this in N - it's unlikely that an app developer will have written code for "only N", but not previous versions. Additionally, if that is the case, it is easy to fix app code to behave the same on all releases (first copy to N32 in Java, then encode to PNG).
The CQ bit was checked by msarett@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by msarett@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by msarett@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by msarett@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by msarett@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from reed@google.com Link to the patchset: https://codereview.chromium.org/2332743003/#ps80001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Disable png encodes from Alpha8, Float16 These don't behave as we would want anyway. They just copy to N32, and then encode. BUG=skia:5616 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2332743003 ========== to ========== Disable png encodes from Alpha8, Float16 These don't behave as we would want anyway. They just copy to N32, and then encode. BUG=skia:5616 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2332743003 Committed: https://skia.googlesource.com/skia/+/029819baa37f2706e6669f97badb20d309568d7c ==========
Message was sent while issue was closed.
Committed patchset #4 (id:80001) as https://skia.googlesource.com/skia/+/029819baa37f2706e6669f97badb20d309568d7c |
