Add the method isOpaque() to SkImage

Add the method isOpaque() to SkImage

BUG=skia:2766
Committed: https://skia.googlesource.com/skia/+/d2a3522503ca0c39829f1bb41f01201d1affdaf6

[Rémi Piotaix, 2014-07-18 20:25:08]

PTAL

[Rémi Piotaix, 2014-07-18 20:34:50]

PTAL




:-D

[reed1, 2014-07-18 20:50:02]

I like the idea, but there may be performance (or correctness) implications for
the codec backend. Adding scroggo and hal to the mix...

https://codereview.chromium.org/406673003/diff/1/src/image/SkImage_Codec.cpp
File src/image/SkImage_Codec.cpp (right):

https://codereview.chromium.org/406673003/diff/1/src/image/SkImage_Codec.cpp#...
src/image/SkImage_Codec.cpp:82: return SkImage::isOpaque();
This will return false, even if the bitmap is opaque.

The absence of a pixelref for this image can just mean that we haven't decoded
the image *yet*

[Rémi Piotaix, 2014-07-18 21:12:51]

Proposition: expose SkBitmap::info() instead of isOpaque().

This can be even more useful. (We can also expose both...)

https://codereview.chromium.org/406673003/diff/1/src/image/SkImage_Codec.cpp
File src/image/SkImage_Codec.cpp (right):

https://codereview.chromium.org/406673003/diff/1/src/image/SkImage_Codec.cpp#...
src/image/SkImage_Codec.cpp:82: return SkImage::isOpaque();
On 2014/07/18 20:50:02, reed1 wrote:
> This will return false, even if the bitmap is opaque.
> 
> The absence of a pixelref for this image can just mean that we haven't decoded
> the image *yet*

Yep.
I made this method const so SkImageDecoder::DecodeMemory() can't be called like
in other methods of SkImage_Codec.

Should i remove the 'const' attribute (or make fBitmap mutable ?)

[scroggo, 2014-07-18 21:39:14]

https://codereview.chromium.org/406673003/diff/1/src/image/SkImage_Codec.cpp
File src/image/SkImage_Codec.cpp (right):

https://codereview.chromium.org/406673003/diff/1/src/image/SkImage_Codec.cpp#...
src/image/SkImage_Codec.cpp:82: return SkImage::isOpaque();
On 2014/07/18 21:12:50, Rémi Piotaix wrote:
> On 2014/07/18 20:50:02, reed1 wrote:
> > This will return false, even if the bitmap is opaque.
> > 
> > The absence of a pixelref for this image can just mean that we haven't
decoded
> > the image *yet*
> 
> Yep.
> I made this method const so SkImageDecoder::DecodeMemory() can't be called
like
> in other methods of SkImage_Codec.
> 
> Should i remove the 'const' attribute (or make fBitmap mutable ?)

It does seem to me that fBitmap is logically mutable.

Might there be a situation where a client wants to know the opacity, but does
not want to necessarily decode yet?

[Rémi Piotaix, 2014-07-22 19:12:35]

And what about exposing SkBitmap::info() instead of isOpaque() ?

This way, we can get more information about the SkImage (now, the only thing we
can get is width and height)

[scroggo, 2014-07-22 19:29:42]

On 2014/07/22 19:12:35, Rémi Piotaix wrote:
> And what about exposing SkBitmap::info() instead of isOpaque() ?
> 
> This way, we can get more information about the SkImage (now, the only thing
we
> can get is width and height)

That seems like a good idea to me - I was surprised to see that SkImage::info()
does
*not* exist, and SkImage does not have an SkImageInfo field. Maybe it's because,
as we've
already noted, some versions of SkImage don't know their info yet (e.g.
SkImage_codec).

reed@ is out right now. We'll need to wait to see what he thinks.

[hal.canary, 2014-07-22 19:33:30]

On 2014/07/22 19:29:42, scroggo wrote:
> That seems like a good idea to me - I was surprised to
> see that SkImage::info() does *not* exist, and SkImage
> does not have an SkImageInfo field.

I thought SkImage is supposed to abstract out details like ColorType/AlphaType.

[Rémi Piotaix, 2014-08-01 18:13:52]

On 2014/07/22 19:33:30, Hal Canary wrote:
> On 2014/07/22 19:29:42, scroggo wrote:
> > That seems like a good idea to me - I was surprised to
> > see that SkImage::info() does *not* exist, and SkImage
> > does not have an SkImageInfo field.
> 
> I thought SkImage is supposed to abstract out details like
ColorType/AlphaType.

So, what do we do?

[reed1, 2014-08-05 13:52:15]

Certainly we began wanting to keep SkImage abstract enough that it could contain
pictures or other non-raster-specific data. We don't have that now, but I think
the jury is still out on its value.

Do we have a use-case for adding isOpaque()? That seems safer, assuming we
actually need it.

[Justin Novosad, 2014-08-05 15:39:23]

On 2014/08/05 13:52:15, reed1 wrote:
> Certainly we began wanting to keep SkImage abstract enough that it could
contain
> pictures or other non-raster-specific data. We don't have that now, but I
think
> the jury is still out on its value.
> 
> Do we have a use-case for adding isOpaque()? That seems safer, assuming we
> actually need it.

We want to use SkImage more broadly in blink to take advantage of copy-on-write.
This is to minimize buffer copies when we do stuff like deferred
canvas-to-canvas draws, or when we create a pattern style from a source canvas.
The underlying rendering code paths have optimizations that kick-in when the
source image is known to be opaque.  Also, it is perfectly fine for the isOpaque
method to have conservative behavior. That is to say that it should return true
if it is guaranteed that the image is opaque, but returning false does not
guarantee anything. Therefore, isOpaque should return false whenever it is
unknown (or expensive to determine) whether the image is opaque. An eventual
SkImage_Picture, for example, could have a virtual overload of isOpaque that
just returns false all the time.

[Rémi Piotaix, 2014-08-06 18:19:09]

Patch 2 reverted: back to isOpaque().

[reed1, 2014-08-06 19:45:03]

Hmmm, the Codec case is interesting, in that we can return different answers
depending on if we have drawn the image or not.

https://codereview.chromium.org/406673003/diff/40001/src/image/SkImage_Codec.cpp
File src/image/SkImage_Codec.cpp (right):

https://codereview.chromium.org/406673003/diff/40001/src/image/SkImage_Codec....
src/image/SkImage_Codec.cpp:81: if (!fBitmap.pixelRef()) {
You can just call fBitmap.isOpaque(), as it returns false for an empty bitmap.

[Rémi Piotaix, 2014-08-08 14:49:15]

https://codereview.chromium.org/406673003/diff/40001/src/image/SkImage_Codec.cpp
File src/image/SkImage_Codec.cpp (right):

https://codereview.chromium.org/406673003/diff/40001/src/image/SkImage_Codec....
src/image/SkImage_Codec.cpp:81: if (!fBitmap.pixelRef()) {
On 2014/08/06 19:45:02, reed1 wrote:
> You can just call fBitmap.isOpaque(), as it returns false for an empty bitmap.

Done.

[reed1, 2014-08-08 18:26:38]

Hal is going to update the codec image code to correctly (lazily) compute its
alphatype w/o forcing a full decode.

Can you add a test for this API? You may be able to just amend one of the
existing ones in tests/*

[Rémi Piotaix, 2014-08-11 23:30:31]

On 2014/08/08 18:26:38, reed1 wrote:
> Hal is going to update the codec image code to correctly (lazily) compute its
> alphatype w/o forcing a full decode.
> 
> Can you add a test for this API? You may be able to just amend one of the
> existing ones in tests/*

Test added.

[reed1, 2014-08-12 13:50:02]

raster parts look fine. brian, does the gpu part of the test look correct /
clear?

[bsalomon, 2014-08-12 15:16:05]

On 2014/08/12 13:50:02, reed1 wrote:
> raster parts look fine. brian, does the gpu part of the test look correct /
> clear?

Seems ok. Is this used anywhere but the test?

[Rémi Piotaix, 2014-08-12 15:17:59]

On 2014/08/12 15:16:05, bsalomon wrote:
> On 2014/08/12 13:50:02, reed1 wrote:
> > raster parts look fine. brian, does the gpu part of the test look correct /
> > clear?
> 
> Seems ok. Is this used anywhere but the test?

It is used here https://codereview.chromium.org/358893002/ in
StaticBitmapImage.cpp

[Rémi Piotaix, 2014-08-19 20:58:16]

Ping ?

[reed1, 2014-08-19 21:21:04]

The CQ bit was checked by reed@google.com

[reed1, 2014-08-19 21:21:05]

lgtm

[commit-bot: I haz the power, 2014-08-19 21:21:47]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/piotaixr@chromium.org/406673003/100001

[commit-bot: I haz the power, 2014-08-19 21:29:15]

Committed patchset #6 (100001) as d2a3522503ca0c39829f1bb41f01201d1affdaf6

[caryclark, 2014-08-20 12:54:01]

A revert of this CL (patchset #6) has been created in
https://codereview.chromium.org/488093002/ by caryclark@google.com.

The reason for reverting is: This breaks the ValGrind build
http://108.170.220.120:10115/builders/Test-Ubuntu12-ShuttleA-GTX550Ti-x86_64-...

which exposes this leak

[01:02:44.642455] ==12875== 
[01:02:44.642530] ==12875== HEAP SUMMARY:
[01:02:45.643727] ==12875==     in use at exit: 140,008,712 bytes in 13,313
blocks
[01:02:45.643846] ==12875==   total heap usage: 38,479,491 allocs, 38,466,178
frees, 827,882,997,105 bytes allocated
[01:02:45.643903] ==12875== 
[01:02:45.643951] ==12875== 340 (88 direct, 252 indirect) bytes in 1 blocks are
definitely lost in loss record 788 of 2,001
[01:02:45.643999] ==12875==    at 0x4C2B1C7: operator new(unsigned long) (in
/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
[01:02:45.644066] ==12875==    by 0x637CCF: SkNewImageFromPixelRef(SkImageInfo
const&, SkPixelRef*, unsigned long) (SkImage_Raster.cpp:218)
[01:02:45.644120] ==12875==    by 0x6373B6: SkNewImageFromBitmap(SkBitmap
const&, bool) (SkImagePriv.cpp:20)
[01:02:45.644167] ==12875==    by 0x638132:
SkSurface_Raster::onNewImageSnapshot() (SkSurface_Raster.cpp:116)
[01:02:45.644213] ==12875==    by 0x637EEE: SkSurface::newImageSnapshot()
(SkSurface_Base.h:91)
[01:02:45.644272] ==12875==    by 0x4B1AFC:
skiatest::ImageIsOpaqueTestClass::onRun(skiatest::Reporter*)
(ImageIsOpaqueTest.cpp:20)
[01:02:45.644319] ==12875==    by 0x4184E0: skiatest::Test::run() (Test.cpp:107)
[01:02:45.644364] ==12875==    by 0x40CD3F: DM::CpuTestTask::draw()
(DMTestTask.cpp:42)
[01:02:45.644422] ==12875==    by 0x40C217: DM::CpuTask::run() (DMTask.cpp:51)
[01:02:45.644467] ==12875==    by 0x40AEF4: SkTThreadPool<void>::Loop(void*)
(SkThreadPool.h:116)
[01:02:45.644514] ==12875==    by 0x73C87A: thread_start(void*)
(SkThreadUtils_pthread.cpp:66)
[01:02:45.644559] ==12875==    by 0x5693E99: start_thread (pthread_create.c:308)
[01:02:45.644605] ==12875==    by 0x6D3FCBC: clone (clone.S:112)
[01:02:45.644651] ==12875== 
[01:02:45.644695] {
[01:02:45.644739]    <insert_a_suppression_name_here>
[01:02:45.644784]    Memcheck:Leak
[01:02:45.644828]    fun:_Znwm
[01:02:45.644874]    fun:_Z22SkNewImageFromPixelRefRK11SkImageInfoP10SkPixelRefm
[01:02:45.644918]    fun:_Z20SkNewImageFromBitmapRK8SkBitmapb
[01:02:45.644962]    fun:_ZN16SkSurface_Raster18onNewImageSnapshotEv
[01:02:45.645009]    fun:_ZN9SkSurface16newImageSnapshotEv
[01:02:45.645054]   
fun:_ZN8skiatest22ImageIsOpaqueTestClass5onRunEPNS_8ReporterE
[01:02:45.645099]    fun:_ZN8skiatest4Test3runEv
[01:02:45.645144]    fun:_ZN2DM11CpuTestTask4drawEv
[01:02:45.645188]    fun:_ZN2DM7CpuTask3runEv
[01:02:45.645232]    fun:_ZN13SkTThreadPoolIvE4LoopEPv
[01:02:45.645297]    fun:_ZL12thread_startPv
[01:02:45.645345]    fun:start_thread
[01:02:45.645394]    fun:clone
[01:02:45.645439] }
[01:02:45.645498] ==12875== 340 (88 direct, 252 indirect) bytes in 1 blocks are
definitely lost in loss record 789 of 2,001
[01:02:45.645544] ==12875==    at 0x4C2B1C7: operator new(unsigned long) (in
/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
[01:02:45.645590] ==12875==    by 0x637CCF: SkNewImageFromPixelRef(SkImageInfo
const&, SkPixelRef*, unsigned long) (SkImage_Raster.cpp:218)
[01:02:45.645636] ==12875==    by 0x6373B6: SkNewImageFromBitmap(SkBitmap
const&, bool) (SkImagePriv.cpp:20)
[01:02:45.645681] ==12875==    by 0x638132:
SkSurface_Raster::onNewImageSnapshot() (SkSurface_Raster.cpp:116)
[01:02:45.645727] ==12875==    by 0x637EEE: SkSurface::newImageSnapshot()
(SkSurface_Base.h:91)
[01:02:45.645771] ==12875==    by 0x4B1B83:
skiatest::ImageIsOpaqueTestClass::onRun(skiatest::Reporter*)
(ImageIsOpaqueTest.cpp:24)
[01:02:45.645829] ==12875==    by 0x4184E0: skiatest::Test::run() (Test.cpp:107)
[01:02:45.645873] ==12875==    by 0x40CD3F: DM::CpuTestTask::draw()
(DMTestTask.cpp:42)
[01:02:45.645927] ==12875==    by 0x40C217: DM::CpuTask::run() (DMTask.cpp:51)
[01:02:45.645972] ==12875==    by 0x40AEF4: SkTThreadPool<void>::Loop(void*)
(SkThreadPool.h:116)
[01:02:45.646026] ==12875==    by 0x73C87A: thread_start(void*)
(SkThreadUtils_pthread.cpp:66)
[01:02:45.646071] ==12875==    by 0x5693E99: start_thread (pthread_create.c:308)
[01:02:45.646124] ==12875==    by 0x6D3FCBC: clone (clone.S:112)
[01:02:45.646169] ==12875== 
[01:02:45.646214] {
[01:02:45.646258]    <insert_a_suppression_name_here>
[01:02:45.646313]    Memcheck:Leak
[01:02:45.646362]    fun:_Znwm
[01:02:45.646407]    fun:_Z22SkNewImageFromPixelRefRK11SkImageInfoP10SkPixelRefm
[01:02:45.646452]    fun:_Z20SkNewImageFromBitmapRK8SkBitmapb
[01:02:45.646498]    fun:_ZN16SkSurface_Raster18onNewImageSnapshotEv
[01:02:45.646544]    fun:_ZN9SkSurface16newImageSnapshotEv
[01:02:45.646589]   
fun:_ZN8skiatest22ImageIsOpaqueTestClass5onRunEPNS_8ReporterE
[01:02:45.646635]    fun:_ZN8skiatest4Test3runEv
[01:02:45.646680]    fun:_ZN2DM11CpuTestTask4drawEv
[01:02:45.646725]    fun:_ZN2DM7CpuTask3runEv
[01:02:45.646770]    fun:_ZN13SkTThreadPoolIvE4LoopEPv
[01:02:45.646815]    fun:_ZL12thread_startPv
[01:02:45.646860]    fun:start_thread
[01:02:45.646904]    fun:clone
[01:02:45.646951] }
[01:02:45.646995] ==12875== 1,168 (176 direct, 992 indirect) bytes in 2 blocks
are definitely lost in loss record 834 of 2,001
[01:02:45.647046] ==12875==    at 0x4C2B1C7: operator new(unsigned long) (in
/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
[01:02:45.647092] ==12875==    by 0x7D975D: SkImage::NewTexture(SkBitmap const&)
(SkImage_Gpu.cpp:89)
[01:02:45.647140] ==12875==    by 0x7C5C3B: SkSurface_Gpu::onNewImageSnapshot()
(SkSurface_Gpu.cpp:66)
[01:02:45.647189] ==12875==    by 0x637EEE: SkSurface::newImageSnapshot()
(SkSurface_Base.h:91)
[01:02:45.647234] ==12875==    by 0x4B1F96:
skiatest::ImageIsOpaqueTest_GPUClass::onRun(skiatest::Reporter*)
(ImageIsOpaqueTest.cpp:45)
[01:02:45.647283] ==12875==    by 0x4184E0: skiatest::Test::run() (Test.cpp:107)
[01:02:45.647328] ==12875==    by 0x40CD09:
DM::GpuTestTask::draw(GrContextFactory*) (DMTestTask.cpp:51)
[01:02:45.647373] ==12875==    by 0x40C27B: DM::GpuTask::run(GrContextFactory&)
(DMTask.cpp:69)
[01:02:45.647420] ==12875==    by 0x40C909:
SkTThreadPool<GrContextFactory>::Loop(void*) (SkThreadPool.h:110)
[01:02:45.647467] ==12875==    by 0x73C87A: thread_start(void*)
(SkThreadUtils_pthread.cpp:66)
[01:02:45.647515] ==12875==    by 0x5693E99: start_thread (pthread_create.c:308)
[01:02:45.647560] ==12875==    by 0x6D3FCBC: clone (clone.S:112)
[01:02:45.647608] ==12875== 
[01:02:45.647654] {
[01:02:45.647700]    <insert_a_suppression_name_here>
[01:02:45.647745]    Memcheck:Leak
[01:02:45.647789]    fun:_Znwm
[01:02:45.647835]    fun:_ZN7SkImage10NewTextureERK8SkBitmap
[01:02:45.647880]    fun:_ZN13SkSurface_Gpu18onNewImageSnapshotEv
[01:02:45.647924]    fun:_ZN9SkSurface16newImageSnapshotEv
[01:02:45.647971]   
fun:_ZN8skiatest26ImageIsOpaqueTest_GPUClass5onRunEPNS_8ReporterE
[01:02:45.648015]    fun:_ZN8skiatest4Test3runEv
[01:02:45.648059]    fun:_ZN2DM11GpuTestTask4drawEP16GrContextFactory
[01:02:45.648108]    fun:_ZN2DM7GpuTask3runER16GrContextFactory
[01:02:45.648154]    fun:_ZN13SkTThreadPoolI16GrContextFactoryE4LoopEPv
[01:02:45.648200]    fun:_ZL12thread_startPv
[01:02:45.648245]    fun:start_thread
[01:02:45.648290]    fun:clone
[01:02:45.648338] }
[01:02:45.648383] ==12875== 1,168 (176 direct, 992 indirect) bytes in 2 blocks
are definitely lost in loss record 835 of 2,001
[01:02:45.648432] ==12875==    at 0x4C2B1C7: operator new(unsigned long) (in
/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
[01:02:45.648478] ==12875==    by 0x7D975D: SkImage::NewTexture(SkBitmap const&)
(SkImage_Gpu.cpp:89)
[01:02:45.648525] ==12875==    by 0x7C5C3B: SkSurface_Gpu::onNewImageSnapshot()
(SkSurface_Gpu.cpp:66)
[01:02:45.648574] ==12875==    by 0x637EEE: SkSurface::newImageSnapshot()
(SkSurface_Base.h:91)
[01:02:45.648620] ==12875==    by 0x4B2030:
skiatest::ImageIsOpaqueTest_GPUClass::onRun(skiatest::Reporter*)
(ImageIsOpaqueTest.cpp:49)
[01:02:45.648668] ==12875==    by 0x4184E0: skiatest::Test::run() (Test.cpp:107)
[01:02:45.648713] ==12875==    by 0x40CD09:
DM::GpuTestTask::draw(GrContextFactory*) (DMTestTask.cpp:51)
[01:02:45.648762] ==12875==    by 0x40C27B: DM::GpuTask::run(GrContextFactory&)
(DMTask.cpp:69)
[01:02:45.648807] ==12875==    by 0x40C909:
SkTThreadPool<GrContextFactory>::Loop(void*) (SkThreadPool.h:110)
[01:02:45.648852] ==12875==    by 0x73C87A: thread_start(void*)
(SkThreadUtils_pthread.cpp:66)
[01:02:45.648900] ==12875==    by 0x5693E99: start_thread (pthread_create.c:308)
[01:02:45.648946] ==12875==    by 0x6D3FCBC: clone (clone.S:112)
[01:02:45.648994] ==12875== 
[01:02:45.649039] {
[01:02:45.649085]    <insert_a_suppression_name_here>
[01:02:45.649130]    Memcheck:Leak
[01:02:45.649174]    fun:_Znwm
[01:02:45.649222]    fun:_ZN7SkImage10NewTextureERK8SkBitmap
[01:02:45.649267]    fun:_ZN13SkSurface_Gpu18onNewImageSnapshotEv
[01:02:45.649322]    fun:_ZN9SkSurface16newImageSnapshotEv
[01:02:45.649367]   
fun:_ZN8skiatest26ImageIsOpaqueTest_GPUClass5onRunEPNS_8ReporterE
[01:02:45.649412]    fun:_ZN8skiatest4Test3runEv
[01:02:45.649460]    fun:_ZN2DM11GpuTestTask4drawEP16GrContextFactory
[01:02:45.649506]    fun:_ZN2DM7GpuTask3runER16GrContextFactory
[01:02:45.649554]    fun:_ZN13SkTThreadPoolI16GrContextFactoryE4LoopEPv
[01:02:45.649599]    fun:_ZL12thread_startPv
[01:02:45.649645]    fun:start_thread
[01:02:45.649690]    fun:clone
[01:02:45.649738] }
[01:02:45.649783] ==12875== LEAK SUMMARY:
[01:02:45.649830] ==12875==    definitely lost: 528 bytes in 6 blocks
[01:02:45.649875] ==12875==    indirectly lost: 2,712 bytes in 23 blocks
[01:02:45.649923] ==12875==      possibly lost: 0 bytes in 0 blocks
[01:02:45.649968] ==12875==    still reachable: 136,805,456 bytes in 12,002
blocks
[01:02:45.650017] ==12875==         suppressed: 3,200,016 bytes in 1,282 blocks
[01:02:45.650062] ==12875== Reachable blocks (those to which a pointer was
found) are not shown.
[01:02:45.650109] ==12875== To see them, rerun with: --leak-check=full
--show-reachable=yes
[01:02:45.650154] ==12875== 
[01:02:45.650199] ==12875== For counts of detected and suppressed errors, rerun
with: -v
[01:02:45.650247] ==12875== ERROR SUMMARY: 4 errors from 4 contexts (suppressed:
44878 from 660)
.