Refactoring in SkImage implementations backed by a SkBitmap

Refactoring in SkImage implementations backed by a SkBitmap

BUG=skia:2948

[Rémi Piotaix, 2014-09-17 20:26:14]

piotaixr@chromium.org changed reviewers:
+ junov@chromium.org, reed@google.com

[Rémi Piotaix, 2014-09-17 21:11:08]

PTAL

Depends on https://codereview.chromium.org/575173002/

[reed1, 2014-09-18 13:07:15]

reed@google.com changed reviewers:
+ bsalomon@google.com

[reed1, 2014-09-18 13:07:17]

https://codereview.chromium.org/577063004/diff/20001/src/image/SkImage_Bitmap...
File src/image/SkImage_BitmapBase.cpp (right):

https://codereview.chromium.org/577063004/diff/20001/src/image/SkImage_Bitmap...
src/image/SkImage_BitmapBase.cpp:15:
SkImage_BitmapBase::SkImage_BitmapBase(const SkBitmap& bitmap) :
INHERITED(bitmap.width(), bitmap.height()), fBitmap(bitmap) {
nit : 100col limit

https://codereview.chromium.org/577063004/diff/20001/src/image/SkImage_Bitmap...
src/image/SkImage_BitmapBase.cpp:18: SkImage_BitmapBase::~SkImage_BitmapBase() {
nit : this is unneeded

https://codereview.chromium.org/577063004/diff/20001/src/image/SkImage_Bitmap...
src/image/SkImage_BitmapBase.cpp:21: const SkBitmap&
SkImage_BitmapBase::getBitmap() const {
nit : this could be in the header

https://codereview.chromium.org/577063004/diff/20001/src/image/SkImage_Gpu.cpp
File src/image/SkImage_Gpu.cpp (right):

https://codereview.chromium.org/577063004/diff/20001/src/image/SkImage_Gpu.cp...
src/image/SkImage_Gpu.cpp:26: //    SkBitmap    fBitmap;
remove?

https://codereview.chromium.org/577063004/diff/20001/src/image/SkImage_Gpu.cp...
src/image/SkImage_Gpu.cpp:60: GrTexture* SkTextureImageGetTexture(SkImage*
image) {
Why do we have this helper?

[bsalomon, 2014-09-18 14:41:51]

https://codereview.chromium.org/577063004/diff/20001/src/image/SkImage_Gpu.cpp
File src/image/SkImage_Gpu.cpp (right):

https://codereview.chromium.org/577063004/diff/20001/src/image/SkImage_Gpu.cp...
src/image/SkImage_Gpu.cpp:15: class SkImage_Gpu : public SkImage_BitmapBase {
Given that we want to remove support for texture-backed SkBitmaps I find it a
bit troubling to bake the bitmap nature of into the inheritance hierarchy.

[bsalomon, 2014-09-18 14:46:36]

On 2014/09/18 14:41:51, bsalomon wrote:
> https://codereview.chromium.org/577063004/diff/20001/src/image/SkImage_Gpu.cpp
> File src/image/SkImage_Gpu.cpp (right):
> 
>
https://codereview.chromium.org/577063004/diff/20001/src/image/SkImage_Gpu.cp...
> src/image/SkImage_Gpu.cpp:15: class SkImage_Gpu : public SkImage_BitmapBase {
> Given that we want to remove support for texture-backed SkBitmaps I find it a
> bit troubling to bake the bitmap nature of into the inheritance hierarchy.
                                            ^ gpu-images

[Rémi Piotaix, 2014-09-18 18:27:15]

https://codereview.chromium.org/577063004/diff/20001/src/image/SkImage_Bitmap...
File src/image/SkImage_BitmapBase.cpp (right):

https://codereview.chromium.org/577063004/diff/20001/src/image/SkImage_Bitmap...
src/image/SkImage_BitmapBase.cpp:15:
SkImage_BitmapBase::SkImage_BitmapBase(const SkBitmap& bitmap) :
INHERITED(bitmap.width(), bitmap.height()), fBitmap(bitmap) {
On 2014/09/18 13:07:16, reed1 wrote:
> nit : 100col limit

Done.

https://codereview.chromium.org/577063004/diff/20001/src/image/SkImage_Bitmap...
src/image/SkImage_BitmapBase.cpp:18: SkImage_BitmapBase::~SkImage_BitmapBase() {
On 2014/09/18 13:07:16, reed1 wrote:
> nit : this is unneeded

Done.

https://codereview.chromium.org/577063004/diff/20001/src/image/SkImage_Bitmap...
src/image/SkImage_BitmapBase.cpp:21: const SkBitmap&
SkImage_BitmapBase::getBitmap() const {
On 2014/09/18 13:07:16, reed1 wrote:
> nit : this could be in the header

Done.

https://codereview.chromium.org/577063004/diff/20001/src/image/SkImage_Gpu.cpp
File src/image/SkImage_Gpu.cpp (right):

https://codereview.chromium.org/577063004/diff/20001/src/image/SkImage_Gpu.cp...
src/image/SkImage_Gpu.cpp:15: class SkImage_Gpu : public SkImage_BitmapBase {
On 2014/09/18 14:41:51, bsalomon wrote:
> Given that we want to remove support for texture-backed SkBitmaps I find it a
> bit troubling to bake the bitmap nature of into the inheritance hierarchy.

Hum, for now this two classes are backed by a SkBitmap, that's why i did that:
there was a lot of code duplication.
Maybe we can make this change: It will still be possible to modify SkImage_Gpu
when the support for tex-based SkBitmaps will be dropped.

https://codereview.chromium.org/577063004/diff/20001/src/image/SkImage_Gpu.cp...
src/image/SkImage_Gpu.cpp:26: //    SkBitmap    fBitmap;
On 2014/09/18 13:07:16, reed1 wrote:
> remove?

Done.

https://codereview.chromium.org/577063004/diff/20001/src/image/SkImage_Gpu.cp...
src/image/SkImage_Gpu.cpp:60: GrTexture* SkTextureImageGetTexture(SkImage*
image) {
On 2014/09/18 13:07:16, reed1 wrote:
> Why do we have this helper?

Uh... dunno.
It is used in SkSurface_Gpu when making copy of the Texture (copy-on-write).
Maybe it would have been cleaner to make SkSurface_Gpu friend of SkImage_Gpu...

[bsalomon, 2014-09-18 18:46:29]

https://codereview.chromium.org/577063004/diff/20001/src/image/SkImage_Gpu.cpp
File src/image/SkImage_Gpu.cpp (right):

https://codereview.chromium.org/577063004/diff/20001/src/image/SkImage_Gpu.cp...
src/image/SkImage_Gpu.cpp:15: class SkImage_Gpu : public SkImage_BitmapBase {
On 2014/09/18 18:27:15, Rémi Piotaix wrote:
> On 2014/09/18 14:41:51, bsalomon wrote:
> > Given that we want to remove support for texture-backed SkBitmaps I find it
a
> > bit troubling to bake the bitmap nature of into the inheritance hierarchy.
> 
> Hum, for now this two classes are backed by a SkBitmap, that's why i did that:
> there was a lot of code duplication.
> Maybe we can make this change: It will still be possible to modify SkImage_Gpu
> when the support for tex-based SkBitmaps will be dropped.

It wasn't obvious at all from the code, but the direction we want to move is for
them to have radically different implementations so I don't think code-sharing
is a good idea. A common base class is place to accumulate more shared
implementation that has to be undone later.

[Rémi Piotaix, 2014-09-18 19:05:26]

On 2014/09/18 18:46:29, bsalomon wrote:
> https://codereview.chromium.org/577063004/diff/20001/src/image/SkImage_Gpu.cpp
> File src/image/SkImage_Gpu.cpp (right):
> 
>
https://codereview.chromium.org/577063004/diff/20001/src/image/SkImage_Gpu.cp...
> src/image/SkImage_Gpu.cpp:15: class SkImage_Gpu : public SkImage_BitmapBase {
> On 2014/09/18 18:27:15, Rémi Piotaix wrote:
> > On 2014/09/18 14:41:51, bsalomon wrote:
> > > Given that we want to remove support for texture-backed SkBitmaps I find
it
> a
> > > bit troubling to bake the bitmap nature of into the inheritance hierarchy.
> > 
> > Hum, for now this two classes are backed by a SkBitmap, that's why i did
that:
> > there was a lot of code duplication.
> > Maybe we can make this change: It will still be possible to modify
SkImage_Gpu
> > when the support for tex-based SkBitmaps will be dropped.
> 
> It wasn't obvious at all from the code, but the direction we want to move is
for
> them to have radically different implementations so I don't think code-sharing
> is a good idea. A common base class is place to accumulate more shared
> implementation that has to be undone later.

If the new implementation will be radically different, It will be necessary to
re-write all the code in SkImage_Gpu, right? Then, it will be the same with or
without this change.

[bsalomon, 2014-09-18 19:24:36]

On 2014/09/18 19:05:26, Rémi Piotaix wrote:
> On 2014/09/18 18:46:29, bsalomon wrote:
> >
https://codereview.chromium.org/577063004/diff/20001/src/image/SkImage_Gpu.cpp
> > File src/image/SkImage_Gpu.cpp (right):
> > 
> >
>
https://codereview.chromium.org/577063004/diff/20001/src/image/SkImage_Gpu.cp...
> > src/image/SkImage_Gpu.cpp:15: class SkImage_Gpu : public SkImage_BitmapBase
{
> > On 2014/09/18 18:27:15, Rémi Piotaix wrote:
> > > On 2014/09/18 14:41:51, bsalomon wrote:
> > > > Given that we want to remove support for texture-backed SkBitmaps I find
> it
> > a
> > > > bit troubling to bake the bitmap nature of into the inheritance
hierarchy.
> > > 
> > > Hum, for now this two classes are backed by a SkBitmap, that's why i did
> that:
> > > there was a lot of code duplication.
> > > Maybe we can make this change: It will still be possible to modify
> SkImage_Gpu
> > > when the support for tex-based SkBitmaps will be dropped.
> > 
> > It wasn't obvious at all from the code, but the direction we want to move is
> for
> > them to have radically different implementations so I don't think
code-sharing
> > is a good idea. A common base class is place to accumulate more shared
> > implementation that has to be undone later.
> 
> If the new implementation will be radically different, It will be necessary to
> re-write all the code in SkImage_Gpu, right? Then, it will be the same with or
> without this change.

It reinforces a usage of SkBitmap that we are trying to remove support for.
Having a common base class means an SkImage_Gpu can masquerade as a
bitmap-backed image to any function that takes a SkImage_BitmapBase.

[reed1, 2014-09-18 20:08:09]

Can we just revert the changes to SkImage_Gpu.cpp?

[Rémi Piotaix, 2014-09-18 20:17:01]

On 2014/09/18 19:24:36, bsalomon wrote:
> On 2014/09/18 19:05:26, Rémi Piotaix wrote:
> > On 2014/09/18 18:46:29, bsalomon wrote:
> > >
> https://codereview.chromium.org/577063004/diff/20001/src/image/SkImage_Gpu.cpp
> > > File src/image/SkImage_Gpu.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/577063004/diff/20001/src/image/SkImage_Gpu.cp...
> > > src/image/SkImage_Gpu.cpp:15: class SkImage_Gpu : public
SkImage_BitmapBase
> {
> > > On 2014/09/18 18:27:15, Rémi Piotaix wrote:
> > > > On 2014/09/18 14:41:51, bsalomon wrote:
> > > > > Given that we want to remove support for texture-backed SkBitmaps I
find
> > it
> > > a
> > > > > bit troubling to bake the bitmap nature of into the inheritance
> hierarchy.
> > > > 
> > > > Hum, for now this two classes are backed by a SkBitmap, that's why i did
> > that:
> > > > there was a lot of code duplication.
> > > > Maybe we can make this change: It will still be possible to modify
> > SkImage_Gpu
> > > > when the support for tex-based SkBitmaps will be dropped.
> > > 
> > > It wasn't obvious at all from the code, but the direction we want to move
is
> > for
> > > them to have radically different implementations so I don't think
> code-sharing
> > > is a good idea. A common base class is place to accumulate more shared
> > > implementation that has to be undone later.
> > 
> > If the new implementation will be radically different, It will be necessary
to
> > re-write all the code in SkImage_Gpu, right? Then, it will be the same with
or
> > without this change.
> 
> It reinforces a usage of SkBitmap that we are trying to remove support for.
> Having a common base class means an SkImage_Gpu can masquerade as a
> bitmap-backed image to any function that takes a SkImage_BitmapBase.

I won't say that it reinforces the usage of a SkBitmap.
The fact that a SkImage_Gpu (and SkImage_Raster) uses a SkBitmap is supposed to
be hidden. Even more, the fact that there are different implementations of
SkImage is hidden behind the SkImage API (SkImage.h)
The inheritance here (with SkImage_BitmapBase) is only behavioral.
I'll just add that, even if it is theoretically possible to have a pointer to a
SkImage_Base internally in Skia, it is *never* used. Why it would be different
for SkImage_BitmapBase?

[Rémi Piotaix, 2014-09-18 20:20:01]

On 2014/09/18 20:08:09, reed1 wrote:
> Can we just revert the changes to SkImage_Gpu.cpp?

I can, but then the SkImage_BitmapBase have no use and i'll have to revert
SkImage_Raster.
In addition, in https://codereview.chromium.org/583453002/, i'll have 2 choices:
- add SkImage_Raster + SkImage_Gpu as friends of SkCanvas
- Add SkImage_Base as friend (instead of SkImage_BitmapBase) and add a
passthrough method is SkImage_Base (which is, IMO, a bad idea as it shows a
weakness in the OO design).

[bsalomon, 2014-09-18 20:35:34]

Deleting the NewEncodedData seems like a good change and we should probably land
that.

On 2014/09/18 20:20:01, Rémi Piotaix wrote:
> On 2014/09/18 20:08:09, reed1 wrote:
> > Can we just revert the changes to SkImage_Gpu.cpp?
> 
> I can, but then the SkImage_BitmapBase have no use and i'll have to revert
> SkImage_Raster.

If you don't revert it then someone else will have to later. I don't see any
value in unifying code that we want differentiate.


> In addition, in https://codereview.chromium.org/583453002/, i'll have 2
choices:
> - add SkImage_Raster + SkImage_Gpu as friends of SkCanvas

I don't think that is necessarily a bad idea.

Alternatively, we could have public asBitmap() asTexture() on SkImage where
SkImage_Gpu returns NULL for the former and SkImage_Raster returns NULL for the
latter. Or if we don't want those in the public we could add those methods to
SkImage_Base and canvas can convert SkImage to SkImage_Base. If it's easier to
discuss alternatives over VC we should set something up for tomorrow.