Creating functions for uploading a mipmapped texture.

Creating functions for uploading a mipmapped texture.

BUG=476416
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1249543003

Committed: https://skia.googlesource.com/skia/+/55f2d2d57f4dd4109aa0c9dab6023373e3b928ec

[cblume, 2015-07-21 10:01:31]

cblume@chromium.org changed reviewers:
+ reed@chromium.org, scroggo@chromium.org

[cblume, 2015-07-21 10:10:11]

I am sorry this took so long.
This is not yet finished and should not be merged in. But it is far enough along
that we can start the CR. This is my first time submitting code to Skia so I
suspect there will be lots of changes in this CR. :) Please be thorough.

Inside SkGr.cpp you will see some TODOs where I had some questions.
1.) Should we use the sketch texture for mipmaps? If so, we'll want to generate
plenty of extra mipmap levels which may go unused.
2.) If I had a bitmap and I get its texture, I do not currently know if that
texture has mipmaps or not. So the return-early won't be exactly what we want, I
believe. How would you like us to handle that? Should the texture have an extra
flag indicating if it has mipmaps or not?
3.) I cherry picked this code back into my Chromium repo but am unsure how best
to test it. I was advised to add a function in SkGr. But I don't see where
Chrome itself is calling anything in SkGr. Is there a good way for me to just
have every bitmap/texture be a mipmapped bitmap/texture so I can make sure it is
working?

Also of important note: In order to pipe the mipmap code through SkGr I added
some functions which take SkMipMaps as parameters. These functions are members
of classes which are visible outside the library. So I ended up moving SkMipMap
and SkCachedData to the include/ directory, making them visible outside the
library as well. I am unsure if this is ideal or if we would prefer to route
things differently.


Thank you for the thorough CR and please let me know if you think this CL is on
the right track.

[scroggo, 2015-07-21 14:44:22]

scroggo@google.com changed reviewers:
+ bsalomon@google.com, scroggo@google.com

[scroggo, 2015-07-21 14:44:23]

I made a few style comments, but this code is not within my area of knowledge.
You'll need a review from someone on Skia's GPU team, so I have added Brian, who
is the head of that team.

https://codereview.chromium.org/1249543003/diff/1/include/gpu/GrTextureProvid...
File include/gpu/GrTextureProvider.h (right):

https://codereview.chromium.org/1249543003/diff/1/include/gpu/GrTextureProvid...
include/gpu/GrTextureProvider.h:12: #include "../core/SkMipMap.h"
Within Skia, we do not fully qualify our includes. (gpu.gyp includes src/core as
an include_dir, so including it directly is fine.)

https://codereview.chromium.org/1249543003/diff/1/src/gpu/GrGpu.cpp
File src/gpu/GrGpu.cpp (right):

https://codereview.chromium.org/1249543003/diff/1/src/gpu/GrGpu.cpp#newcode67
src/gpu/GrGpu.cpp:67: static bool check_texture_creation_params(const GrCaps*
caps, const GrSurfaceDesc& desc,
nit: typically we use const& for parameters which are mandatory.

More importantly, can you add comments?

https://codereview.chromium.org/1249543003/diff/1/src/gpu/GrGpu.cpp#newcode68
src/gpu/GrGpu.cpp:68: bool& isRT) {
nit: typically we use pointers for parameters which may be changed. So this
should be:

bool* isRT

https://codereview.chromium.org/1249543003/diff/1/src/gpu/GrGpu.cpp#newcode103
src/gpu/GrGpu.cpp:103: if (textureCreationParamsValid == false) {
nit: typically we just say

if (!textureCreationParamsValid) {

https://codereview.chromium.org/1249543003/diff/1/src/gpu/SkGr.cpp
File src/gpu/SkGr.cpp (right):

https://codereview.chromium.org/1249543003/diff/1/src/gpu/SkGr.cpp#newcode655
src/gpu/SkGr.cpp:655: // TODO: What if the bitmap already contains a
non-mitmapped texture?
non-mipmapped?

https://codereview.chromium.org/1249543003/diff/1/src/gpu/SkGr.cpp#newcode657
src/gpu/SkGr.cpp:657: // TODO: Add stretching. After reading get_stretch(), it
appears small
nit: this should be a four space indent

[cblume, 2015-07-21 23:07:30]

https://codereview.chromium.org/1249543003/diff/1/src/gpu/GrGpu.cpp
File src/gpu/GrGpu.cpp (right):

https://codereview.chromium.org/1249543003/diff/1/src/gpu/GrGpu.cpp#newcode67
src/gpu/GrGpu.cpp:67: static bool check_texture_creation_params(const GrCaps*
caps, const GrSurfaceDesc& desc,
On 2015/07/21 14:44:23, scroggo wrote:
> nit: typically we use const& for parameters which are mandatory.
> 
> More importantly, can you add comments?

Done.

https://codereview.chromium.org/1249543003/diff/1/src/gpu/GrGpu.cpp#newcode68
src/gpu/GrGpu.cpp:68: bool& isRT) {
On 2015/07/21 14:44:23, scroggo wrote:
> nit: typically we use pointers for parameters which may be changed. So this
> should be:
> 
> bool* isRT

Done.

https://codereview.chromium.org/1249543003/diff/1/src/gpu/GrGpu.cpp#newcode103
src/gpu/GrGpu.cpp:103: if (textureCreationParamsValid == false) {
On 2015/07/21 14:44:23, scroggo wrote:
> nit: typically we just say
> 
> if (!textureCreationParamsValid) {

Done.

https://codereview.chromium.org/1249543003/diff/1/src/gpu/SkGr.cpp
File src/gpu/SkGr.cpp (right):

https://codereview.chromium.org/1249543003/diff/1/src/gpu/SkGr.cpp#newcode655
src/gpu/SkGr.cpp:655: // TODO: What if the bitmap already contains a
non-mitmapped texture?
On 2015/07/21 14:44:23, scroggo wrote:
> non-mipmapped?

I mean what if someone calls creates a regular texture from a bitmap (this
texture does not have any extra mipmap levels). Then they call this function
passing in that same bitmap.

GrRefCachedBitmapTexture and this function both exit early if the texture was
already created. It just returns that existing texture.

But if the texture was created without any extra mipmap levels, it would be off
to exit early with a non-mipmapped texture.

https://codereview.chromium.org/1249543003/diff/1/src/gpu/SkGr.cpp#newcode657
src/gpu/SkGr.cpp:657: // TODO: Add stretching. After reading get_stretch(), it
appears small
On 2015/07/21 14:44:23, scroggo wrote:
> nit: this should be a four space indent

Done.

[bsalomon, 2015-07-22 17:21:10]

https://codereview.chromium.org/1249543003/diff/20001/include/gpu/GrTexturePr...
File include/gpu/GrTextureProvider.h (right):

https://codereview.chromium.org/1249543003/diff/20001/include/gpu/GrTexturePr...
include/gpu/GrTextureProvider.h:33: /**
What do you think about combining createTexture and createMipMappedTexture into
one function that takes a mip level iterator?

Something like

GrMipIterator {
    virtual int levelCount();
    virtual void* getLevel(size_t* rowBytes);
}

?

And then we could still have a createTexture() that takes a void*/size_t and
calls the "real" version with a single level. All the downstream calls wouldn't
need to impls, though.

[cblume, 2015-07-22 18:32:40]

https://codereview.chromium.org/1249543003/diff/20001/include/gpu/GrTexturePr...
File include/gpu/GrTextureProvider.h (right):

https://codereview.chromium.org/1249543003/diff/20001/include/gpu/GrTexturePr...
include/gpu/GrTextureProvider.h:33: /**
On 2015/07/22 17:21:10, bsalomon wrote:
> What do you think about combining createTexture and createMipMappedTexture
into
> one function that takes a mip level iterator?
> 
> Something like
> 
> GrMipIterator {
>     virtual int levelCount();
>     virtual void* getLevel(size_t* rowBytes);
> }
> 
> ?
> 
> And then we could still have a createTexture() that takes a void*/size_t and
> calls the "real" version with a single level. All the downstream calls
wouldn't
> need to impls, though.

I like that idea.

In general, I am in favor of having one function instead of two. Elsewhere in
this CL you can see some of the functions I refactored to take an SkTArray of
mip levels. And the places with only 1 mip level only populate 1 element of that
array. So I very much think that would be a good idea.

For that to work I beliebe we'll have all the piping take the mip iterator or
mip array and only the topmost layer would continue to take void*/size_t. Is
that what you were thinking?

[bsalomon, 2015-07-22 18:57:07]

On 2015/07/22 18:32:40, cblume wrote:
>
https://codereview.chromium.org/1249543003/diff/20001/include/gpu/GrTexturePr...
> File include/gpu/GrTextureProvider.h (right):
> 
>
https://codereview.chromium.org/1249543003/diff/20001/include/gpu/GrTexturePr...
> include/gpu/GrTextureProvider.h:33: /**
> On 2015/07/22 17:21:10, bsalomon wrote:
> > What do you think about combining createTexture and createMipMappedTexture
> into
> > one function that takes a mip level iterator?
> > 
> > Something like
> > 
> > GrMipIterator {
> >     virtual int levelCount();
> >     virtual void* getLevel(size_t* rowBytes);
> > }
> > 
> > ?
> > 
> > And then we could still have a createTexture() that takes a void*/size_t and
> > calls the "real" version with a single level. All the downstream calls
> wouldn't
> > need to impls, though.
> 
> I like that idea.
> 
> In general, I am in favor of having one function instead of two. Elsewhere in
> this CL you can see some of the functions I refactored to take an SkTArray of
> mip levels. And the places with only 1 mip level only populate 1 element of
that
> array. So I very much think that would be a good idea.
> 
> For that to work I beliebe we'll have all the piping take the mip iterator or
> mip array and only the topmost layer would continue to take void*/size_t. Is
> that what you were thinking?

Yup, that's exactly what I was thinking.

[cblume, 2015-07-23 18:18:59]

https://codereview.chromium.org/1249543003/diff/20001/src/gpu/gl/GrGLGpu.cpp
File src/gpu/gl/GrGLGpu.cpp (right):

https://codereview.chromium.org/1249543003/diff/20001/src/gpu/gl/GrGLGpu.cpp#...
src/gpu/gl/GrGLGpu.cpp:798: SkToInt(dataSize),
I think this may be a bug. Each mip level probably has its own size, right? I
shouldn't be reusing one size.

[cblume, 2015-07-24 23:49:03]

I have updated the style. I think I capitalized with "Mipmap" which is
inconsistent with "SkMipMap". Should I change it to "MipMap"?

Also I use some C++11 features like auto and nullptr. I'm not sure if Skia likes
or dislikes that. Let me know and I'll flip it around to what you prefer.

Thanks!

https://codereview.chromium.org/1249543003/diff/20001/include/gpu/GrTexturePr...
File include/gpu/GrTextureProvider.h (right):

https://codereview.chromium.org/1249543003/diff/20001/include/gpu/GrTexturePr...
include/gpu/GrTextureProvider.h:33: /**
On 2015/07/22 17:21:10, bsalomon wrote:
> What do you think about combining createTexture and createMipMappedTexture
into
> one function that takes a mip level iterator?
> 
> Something like
> 
> GrMipIterator {
>     virtual int levelCount();
>     virtual void* getLevel(size_t* rowBytes);
> }
> 
> ?
> 
> And then we could still have a createTexture() that takes a void*/size_t and
> calls the "real" version with a single level. All the downstream calls
wouldn't
> need to impls, though.

Thinking a bit more about this...ideally mipmapped and non-mipmapped textures
textures behave the same and we can just have one function for each piece of the
pipeline.

However, there may be cases where it is not yet clear that we can treat them the
same.

For example, should the scratch texture have mipmaps? If so we'll need it to
have >= the number of mipmap levels we plan to allow as a max. That also means
extra memory usage (although, maybe not much?).

It may also mean changing the stretch code to also stretch the mipmaps but that
is do-able. In fact, I think we'll end up needing to do it. So that is okay.

What do you think?

[bsalomon, 2015-07-27 13:14:26]

On 2015/07/24 23:49:03, cblume wrote:
> I have updated the style. I think I capitalized with "Mipmap" which is
> inconsistent with "SkMipMap". Should I change it to "MipMap"?

Let's go with MipMap just to be consistent. 

> 
> Also I use some C++11 features like auto and nullptr. I'm not sure if Skia
likes
> or dislikes that. Let me know and I'll flip it around to what you prefer.
> 
> Thanks!
> 
>
https://codereview.chromium.org/1249543003/diff/20001/include/gpu/GrTexturePr...
> File include/gpu/GrTextureProvider.h (right):
> 
>
https://codereview.chromium.org/1249543003/diff/20001/include/gpu/GrTexturePr...
> include/gpu/GrTextureProvider.h:33: /**
> On 2015/07/22 17:21:10, bsalomon wrote:
> > What do you think about combining createTexture and createMipMappedTexture
> into
> > one function that takes a mip level iterator?
> > 
> > Something like
> > 
> > GrMipIterator {
> >     virtual int levelCount();
> >     virtual void* getLevel(size_t* rowBytes);
> > }
> > 
> > ?
> > 
> > And then we could still have a createTexture() that takes a void*/size_t and
> > calls the "real" version with a single level. All the downstream calls
> wouldn't
> > need to impls, though.
> 
> Thinking a bit more about this...ideally mipmapped and non-mipmapped textures
> textures behave the same and we can just have one function for each piece of
the
> pipeline.
> 
> However, there may be cases where it is not yet clear that we can treat them
the
> same.
> 
> For example, should the scratch texture have mipmaps? If so we'll need it to
> have >= the number of mipmap levels we plan to allow as a max. That also means
> extra memory usage (although, maybe not much?).

This alread an issue since a bitmap texture can be recycled as a scratch texture
after mip maps were added. I think we can address this separately by making the
mip map status part of the scratch key.


> 
> It may also mean changing the stretch code to also stretch the mipmaps but
that
> is do-able. In fact, I think we'll end up needing to do it. So that is okay.

The stretch code is used for GPUs that don't support tiling on NPOT textures.
They also don't support MIP maps on NPOT textures. Skia has decided that if we
can't support MIP maps on arbitrarily sized textures we just won't support them
at all. So fortunately I think we can just ignore the stretching code for MIP
maps.

[reed1, 2015-07-27 21:18:09]

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

[reed1, 2015-07-27 21:18:09]

can we keep SkCachedData and SkMipMap private (in the src/core directories)?

[scroggo, 2015-08-06 18:56:41]

scroggo@google.com changed reviewers:
- scroggo@chromium.org, scroggo@google.com

[cblume, 2015-08-14 21:45:56]

On 2015/07/27 13:14:26, bsalomon wrote:
> On 2015/07/24 23:49:03, cblume wrote:
> > I have updated the style. I think I capitalized with "Mipmap" which is
> > inconsistent with "SkMipMap". Should I change it to "MipMap"?
> 
> Let's go with MipMap just to be consistent. 

Done.

> I think we can address this separately by making the
> mip map status part of the scratch key.

Not done yet. Haven't looked into the scratch key. I may soon come with some
questions about this.

> So fortunately I think we can just ignore the stretching code for MIP
> maps.

Sounds good. I haven't yet removed the TODO for this but I will when updating
the scratch key stuff.

[cblume, 2015-08-14 21:48:51]

On 2015/07/27 21:18:09, reed1 wrote:
> can we keep SkCachedData and SkMipMap private (in the src/core directories)?

Done.
In order to do this I moved the definition of the SkDiscardableMemoryFactoryProc
from SkMipMap.h to SkMipMapLevel.h. This allowed me to put SkMipMap.h and
SkCachedData.h back where they were. Eventually, we may want to allow the caller
to provide the mipmap levels themselves (if say they are loading a texture which
already has them generated). And so it might make sense to have SkMipMapLevel.h
exposed eventually (even though right now it doesn't need to be).

[cblume, 2015-08-14 21:56:56]

This now uses only 1 path for compressed and 1 path for uncompressed. It does
this by taking a SkTArray<SkMipMapLevel>. Texture without mipmaps simply only
contain one element in that TArray.

Right now, SkMipMapLevel is very very dumb about its memory. It assumes the
caller will still own the memory. This happens to work right now since
SkMipMapLevel is only used in blocking functions. But I don't feel very
comfortable with it. Maybe I could make it a reference instead? That is slightly
closer to the style guide. However, there is some code which alters what is
being pointed to so it can realloc. A reference wouldn't allow this.

I am considering a more modern C++11 approach to the memory management.

And I'm also considering altering SkMipMap a bit since it has a nested class for
mipmap levels. But that can be a separate issue.

https://codereview.chromium.org/1249543003/diff/20001/src/gpu/gl/GrGLGpu.cpp
File src/gpu/gl/GrGLGpu.cpp (right):

https://codereview.chromium.org/1249543003/diff/20001/src/gpu/gl/GrGLGpu.cpp#...
src/gpu/gl/GrGLGpu.cpp:798: SkToInt(dataSize),
On 2015/07/23 18:18:59, cblume wrote:
> I think this may be a bug. Each mip level probably has its own size, right? I
> shouldn't be reusing one size.

Done.

[bsalomon, 2015-08-26 18:30:11]

Sorry for the slow review. Still need to look in detail at GrGLGpu. Also in the
future feel free to yell at me if I don't respond quickly! :)

https://codereview.chromium.org/1249543003/diff/80001/src/gpu/GrGpu.cpp
File src/gpu/GrGpu.cpp (right):

https://codereview.chromium.org/1249543003/diff/80001/src/gpu/GrGpu.cpp#newco...
src/gpu/GrGpu.cpp:145: /* TODO: I am unsure how to use a scratch texture well
with mipmaps
I think we should just include mip status in the scratch key and change the key
if mips get added lazily to a non-mipped texture.

https://codereview.chromium.org/1249543003/diff/80001/src/gpu/GrTextureProvid...
File src/gpu/GrTextureProvider.cpp (right):

https://codereview.chromium.org/1249543003/diff/80001/src/gpu/GrTextureProvid...
src/gpu/GrTextureProvider.cpp:48: return createTexture(desc, budgeted, texels);
style nit, we prepend method calls with "this->"

https://codereview.chromium.org/1249543003/diff/80001/src/gpu/SkGr.cpp
File src/gpu/SkGr.cpp (right):

https://codereview.chromium.org/1249543003/diff/80001/src/gpu/SkGr.cpp#newcod...
src/gpu/SkGr.cpp:654: // TODO: What if the bitmap already contains a
non-mipmapped texture?
Maybe just preserve the existing glGenMipmaps()? texture-backed SkBitmaps are
going away anyway. Also we can have content generated on the GPU that needs to
be mipped, seems like calling the driver is right there.

https://codereview.chromium.org/1249543003/diff/80001/src/gpu/gl/GrGLGpu.cpp
File src/gpu/gl/GrGLGpu.cpp (right):

https://codereview.chromium.org/1249543003/diff/80001/src/gpu/gl/GrGLGpu.cpp#...
src/gpu/gl/GrGLGpu.cpp:1418: (this, sbDesc, width, height, samples, format)));
nit wrap at 100 col

[cblume, 2015-08-26 18:58:00]

I still feel a tad uncomfortable with how the new SkMipMapLevel does not ensure
its memory is safe. Would a ref counted pointer be acceptable?

https://codereview.chromium.org/1249543003/diff/80001/src/gpu/GrGpu.cpp
File src/gpu/GrGpu.cpp (right):

https://codereview.chromium.org/1249543003/diff/80001/src/gpu/GrGpu.cpp#newco...
src/gpu/GrGpu.cpp:145: /* TODO: I am unsure how to use a scratch texture well
with mipmaps
On 2015/08/26 18:30:11, bsalomon wrote:
> I think we should just include mip status in the scratch key and change the
key
> if mips get added lazily to a non-mipped texture.

Thinking out loud: A bool of "is mipmapped" may not be enough. If someone wanted
a scratch texture with 5 mipmap levels but they are provided a scratch texture
with only 4 then there will be a problem. So maybe mipmap level count is the
thing we need to add to the scratch texture. Maybe this also means we cannot
preallocate the scratch texture mipmap memory, which would be a significant
slowdown any time we need to add more levels.

https://codereview.chromium.org/1249543003/diff/80001/src/gpu/GrTextureProvid...
File src/gpu/GrTextureProvider.cpp (right):

https://codereview.chromium.org/1249543003/diff/80001/src/gpu/GrTextureProvid...
src/gpu/GrTextureProvider.cpp:48: return createTexture(desc, budgeted, texels);
On 2015/08/26 18:30:11, bsalomon wrote:
> style nit, we prepend method calls with "this->"

Done.

https://codereview.chromium.org/1249543003/diff/80001/src/gpu/SkGr.cpp
File src/gpu/SkGr.cpp (right):

https://codereview.chromium.org/1249543003/diff/80001/src/gpu/SkGr.cpp#newcod...
src/gpu/SkGr.cpp:654: // TODO: What if the bitmap already contains a
non-mipmapped texture?
On 2015/08/26 18:30:11, bsalomon wrote:
> Maybe just preserve the existing glGenMipmaps()? texture-backed SkBitmaps are
> going away anyway. Also we can have content generated on the GPU that needs to
> be mipped, seems like calling the driver is right there.

Hrmmm the point of all this was to try to remove the call to glGenMipmaps(). I
suppose for this to matter the caller would need to be aware if the SkBitmap
they are passing is backed by a mipmapped texture or not so they can prevent the
call to glGenMipmaps(). I'm not sure if that is awkward for the caller to do.

How are texture-backed SkBitmaps going away? Maybe there is a better solution
using the new system.

GPU-generated content that needs to be mipped will be an interesting problem.
After a bit more research, it sounds like mipmaps may currently be generated by
rendering to the mipmap level. I would imagine it would be difficult to beat
that as far as speed but it may still cause jank on the GPU thread? Reading the
texture back out to the GPU would likely be worse.

https://codereview.chromium.org/1249543003/diff/80001/src/gpu/gl/GrGLGpu.cpp
File src/gpu/gl/GrGLGpu.cpp (right):

https://codereview.chromium.org/1249543003/diff/80001/src/gpu/gl/GrGLGpu.cpp#...
src/gpu/gl/GrGLGpu.cpp:1418: (this, sbDesc, width, height, samples, format)));
On 2015/08/26 18:30:11, bsalomon wrote:
> nit wrap at 100 col

Done.

[bsalomon, 2015-08-27 13:22:49]

https://codereview.chromium.org/1249543003/diff/80001/src/gpu/GrGpu.cpp
File src/gpu/GrGpu.cpp (right):

https://codereview.chromium.org/1249543003/diff/80001/src/gpu/GrGpu.cpp#newco...
src/gpu/GrGpu.cpp:145: /* TODO: I am unsure how to use a scratch texture well
with mipmaps
On 2015/08/26 18:58:00, cblume wrote:
> On 2015/08/26 18:30:11, bsalomon wrote:
> > I think we should just include mip status in the scratch key and change the
> key
> > if mips get added lazily to a non-mipped texture.
> 
> Thinking out loud: A bool of "is mipmapped" may not be enough. If someone
wanted
> a scratch texture with 5 mipmap levels but they are provided a scratch texture
> with only 4 then there will be a problem. So maybe mipmap level count is the
> thing we need to add to the scratch texture. Maybe this also means we cannot
> preallocate the scratch texture mipmap memory, which would be a significant
> slowdown any time we need to add more levels.

Why don't we just require either just a base level or a full pyramid? The upper
levels are so small compared to the original that I don't know that it is worth
any complexity to save a tiny amount of space.

https://codereview.chromium.org/1249543003/diff/80001/src/gpu/SkGr.cpp
File src/gpu/SkGr.cpp (right):

https://codereview.chromium.org/1249543003/diff/80001/src/gpu/SkGr.cpp#newcod...
src/gpu/SkGr.cpp:654: // TODO: What if the bitmap already contains a
non-mipmapped texture?
On 2015/08/26 18:58:00, cblume wrote:
> On 2015/08/26 18:30:11, bsalomon wrote:
> > Maybe just preserve the existing glGenMipmaps()? texture-backed SkBitmaps
are
> > going away anyway. Also we can have content generated on the GPU that needs
to
> > be mipped, seems like calling the driver is right there.
> 
> Hrmmm the point of all this was to try to remove the call to glGenMipmaps(). I
> suppose for this to matter the caller would need to be aware if the SkBitmap
> they are passing is backed by a mipmapped texture or not so they can prevent
the
> call to glGenMipmaps(). I'm not sure if that is awkward for the caller to do.
> 
> How are texture-backed SkBitmaps going away? Maybe there is a better solution
> using the new system.
> 
> GPU-generated content that needs to be mipped will be an interesting problem.
> After a bit more research, it sounds like mipmaps may currently be generated
by
> rendering to the mipmap level. I would imagine it would be difficult to beat
> that as far as speed but it may still cause jank on the GPU thread? Reading
the
> texture back out to the GPU would likely be worse.

The GrTexture tracks whether it's MIPs are "dirty" (rendered to) and only regens
if they are. SkBitmaps that are already explicitly texture backed (meaning they
actually own a texture rather than simply are associated with one in the cache)
represent content that was generated on the GPU. So I think glGenMipMipmaps is
probably a better solution than readpixels/cpu-gen/upload.

SkBitmaps backed by GrTextures are problematic because the interfaces on
SkBitmap was really designed around CPU-accessible pixels. Also, they make no
distinctions between sources and destinations. The replacement abstractions are
SkSurface and SkImage. SkImages are sources for draws and I think all the same
reasoning applies there (generate on CPU for sw-backed SkImages, glGenMipMaps
for inherently texture backed SkImages).

[bsalomon, 2015-09-15 13:14:11]

Two serious comments:

I do think you need to hook up the invalidator. It is used to immediately delete
(or make recyclable) a cached texture for a SkPixelRef when the pixel ref is
deleted or its genID is modified (because it's pixels are modified). Otherwise
we have a wait for the texture to age out of th cache even though it's
GrUniqueKey will never be searched for again.

The other thing, is I think we need to preserve the GenMipMaps code path for
rendered content (as opposed to uploaded content) or come up with an alternate
way of creating MIP maps for such content.


And two non-inline nits:

There are a bunch of places that exceed the 100 col limit. 

In Skia-land we aren't (yet?) using auto so widely. I'd prefer to use explicit
types until the team has some consensus about adopting auto in more cases.

https://codereview.chromium.org/1249543003/diff/240001/include/gpu/GrSurface.h
File include/gpu/GrSurface.h (right):

https://codereview.chromium.org/1249543003/diff/240001/include/gpu/GrSurface....
include/gpu/GrSurface.h:119: * This is so older code which currently uses this
function signature will
I'd take off the thing about older code. I expect even future callers would
continue to use this as a convenience for cases where they know the texture only
has a base level.

https://codereview.chromium.org/1249543003/diff/240001/include/gpu/GrTexture.h
File include/gpu/GrTexture.h (left):

https://codereview.chromium.org/1249543003/diff/240001/include/gpu/GrTexture....
include/gpu/GrTexture.h:54: void dirtyMipMaps(bool mipMapsDirty);
What happens if we draw to the base of a texture that has mips? Don't we still
need to regenerate in that case? Maybe we need a test for this case?

https://codereview.chromium.org/1249543003/diff/240001/src/gpu/GrContext.cpp
File src/gpu/GrContext.cpp (right):

https://codereview.chromium.org/1249543003/diff/240001/src/gpu/GrContext.cpp#...
src/gpu/GrContext.cpp:329: SkTArray<SkMipMapLevel> texelsCopy(texels);
Why do we need to copy the mip array?

https://codereview.chromium.org/1249543003/diff/240001/src/gpu/GrGpu.h
File src/gpu/GrGpu.h (right):

https://codereview.chromium.org/1249543003/diff/240001/src/gpu/GrGpu.h#newcode98
src/gpu/GrGpu.h:98: * This is so older code which currently uses this function
signature will
-older

https://codereview.chromium.org/1249543003/diff/240001/src/gpu/GrGpu.h#newcod...
src/gpu/GrGpu.h:259: * This is so older code which currently uses this function
signature will
-older

https://codereview.chromium.org/1249543003/diff/240001/src/gpu/GrTexture.cpp
File src/gpu/GrTexture.cpp (right):

https://codereview.chromium.org/1249543003/diff/240001/src/gpu/GrTexture.cpp#...
src/gpu/GrTexture.cpp:100: SkASSERT(log2(kLast_GrPixelConfig) <= 5);
Can you use sk_float_log2? It looks like it works around some issue on android.

https://codereview.chromium.org/1249543003/diff/240001/src/gpu/GrTexture.cpp#...
src/gpu/GrTexture.cpp:107: builder[1] = desc.fConfig | desc.fIsMipMapped << 5 |
(desc.fSampleCnt << 6) | (flags << 14) | (origin << 24);
100 col wrap

https://codereview.chromium.org/1249543003/diff/240001/src/gpu/gl/GrGLGpu.cpp
File src/gpu/gl/GrGLGpu.cpp (right):

https://codereview.chromium.org/1249543003/diff/240001/src/gpu/gl/GrGLGpu.cpp...
src/gpu/gl/GrGLGpu.cpp:867: static void restore_gl_state(const GrGLInterface&
interface, const GrGLCaps& caps,
maybe give this a narrower name? e.g. restore_pixelstore_state?

https://codereview.chromium.org/1249543003/diff/240001/tests/GrTextureMipMapI...
File tests/GrTextureMipMapInvalidationTest.cpp (left):

https://codereview.chromium.org/1249543003/diff/240001/tests/GrTextureMipMapI...
tests/GrTextureMipMapInvalidationTest.cpp:44: // Painting with downscale and
medium filter quality should result in mipmap creation
This still has to work...

[cblume, 2015-09-16 18:40:46]

https://codereview.chromium.org/1249543003/diff/240001/include/gpu/GrSurface.h
File include/gpu/GrSurface.h (right):

https://codereview.chromium.org/1249543003/diff/240001/include/gpu/GrSurface....
include/gpu/GrSurface.h:119: * This is so older code which currently uses this
function signature will
On 2015/09/15 13:14:10, bsalomon wrote:
> I'd take off the thing about older code. I expect even future callers would
> continue to use this as a convenience for cases where they know the texture
only
> has a base level.

I've updated all the shim comments. :)

https://codereview.chromium.org/1249543003/diff/240001/include/gpu/GrTexture.h
File include/gpu/GrTexture.h (left):

https://codereview.chromium.org/1249543003/diff/240001/include/gpu/GrTexture....
include/gpu/GrTexture.h:54: void dirtyMipMaps(bool mipMapsDirty);
On 2015/09/15 13:14:10, bsalomon wrote:
> What happens if we draw to the base of a texture that has mips? Don't we still
> need to regenerate in that case? Maybe we need a test for this case?

You are right. Last night, while writing this, I thought "the user will provide
all the mipmap levels." But that isn't the case for rendered-to content. We had
already mentioned that and I forgot last night.

https://codereview.chromium.org/1249543003/diff/240001/src/gpu/GrContext.cpp
File src/gpu/GrContext.cpp (right):

https://codereview.chromium.org/1249543003/diff/240001/src/gpu/GrContext.cpp#...
src/gpu/GrContext.cpp:329: SkTArray<SkMipMapLevel> texelsCopy(texels);
On 2015/09/15 13:14:11, bsalomon wrote:
> Why do we need to copy the mip array?

Further down there is a call to GrSurfacePriv::AdjustWritePixelParams inside a
loop over each mipmap level. That call changes the mipmap level texel pointer to
adjust for texture offset (maybe we are writing somewhere to the center of the
texture, not to the top left).

The array being copied is filled with lightweight objects (a pointer to the
texel data for that level and the row bytes for that level). The actual texel
data is not being copied.

https://codereview.chromium.org/1249543003/diff/240001/src/gpu/GrTexture.cpp
File src/gpu/GrTexture.cpp (right):

https://codereview.chromium.org/1249543003/diff/240001/src/gpu/GrTexture.cpp#...
src/gpu/GrTexture.cpp:100: SkASSERT(log2(kLast_GrPixelConfig) <= 5);
On 2015/09/15 13:14:11, bsalomon wrote:
> Can you use sk_float_log2? It looks like it works around some issue on
android.

Good catch.

https://codereview.chromium.org/1249543003/diff/240001/src/gpu/GrTexture.cpp#...
src/gpu/GrTexture.cpp:107: builder[1] = desc.fConfig | desc.fIsMipMapped << 5 |
(desc.fSampleCnt << 6) | (flags << 14) | (origin << 24);
On 2015/09/15 13:14:11, bsalomon wrote:
> 100 col wrap

Done.

https://codereview.chromium.org/1249543003/diff/240001/src/gpu/gl/GrGLGpu.cpp
File src/gpu/gl/GrGLGpu.cpp (right):

https://codereview.chromium.org/1249543003/diff/240001/src/gpu/gl/GrGLGpu.cpp...
src/gpu/gl/GrGLGpu.cpp:867: static void restore_gl_state(const GrGLInterface&
interface, const GrGLCaps& caps,
On 2015/09/15 13:14:11, bsalomon wrote:
> maybe give this a narrower name? e.g. restore_pixelstore_state?

Oh wow, that is a much better name. Done.

[cblume, 2015-09-19 08:59:44]

I was able to run skdiff and had only a small amount of images with only
slightly different results. This might be expected since the way we generate
mipmaps from within Skia will be different from how a video card's drivers
generate mipmaps.

That said, I disabled an assert that I thought would be valuable. It is late. I
will investigate more when I have more energy.

[bsalomon, 2015-09-30 18:01:30]

mostly looks good, small stuff noted inline.

https://codereview.chromium.org/1249543003/diff/450001/include/gpu/GrContext.h
File include/gpu/GrContext.h (right):

https://codereview.chromium.org/1249543003/diff/450001/include/gpu/GrContext....
include/gpu/GrContext.h:267: *                      element and it contains
nullptr fTexels, texture data is
Why one element and not just a 0 element array?

https://codereview.chromium.org/1249543003/diff/450001/src/gpu/GrContext.cpp
File src/gpu/GrContext.cpp (right):

https://codereview.chromium.org/1249543003/diff/450001/src/gpu/GrContext.cpp#...
src/gpu/GrContext.cpp:348: currentMipWidth = std::max(1, currentMipWidth / 2);
SkTMax

https://codereview.chromium.org/1249543003/diff/450001/src/gpu/GrContext.cpp#...
src/gpu/GrContext.cpp:349: currentMipHeight = std::max(1, currentMipHeight / 2);
SkTMax

https://codereview.chromium.org/1249543003/diff/450001/src/gpu/GrContext.cpp#...
src/gpu/GrContext.cpp:435: currentWidth = std::max(1, currentWidth / 2);
SkTMax

https://codereview.chromium.org/1249543003/diff/450001/src/gpu/GrContext.cpp#...
src/gpu/GrContext.cpp:436: currentHeight = std::max(1, currentHeight / 2);
SkTMax

https://codereview.chromium.org/1249543003/diff/450001/src/gpu/GrContext.cpp#...
src/gpu/GrContext.cpp:475: currentMipWidth = std::max(1, currentMipWidth / 2);
SkTMax

https://codereview.chromium.org/1249543003/diff/450001/src/gpu/GrContext.cpp#...
src/gpu/GrContext.cpp:476: currentMipHeight = std::max(1, currentMipHeight / 2);
SkTMax

https://codereview.chromium.org/1249543003/diff/450001/src/gpu/GrContext.cpp#...
src/gpu/GrContext.cpp:489: if (width < 0 || height < 0) {
Why not <=?

https://codereview.chromium.org/1249543003/diff/450001/src/gpu/GrGpu.cpp
File src/gpu/GrGpu.cpp (right):

https://codereview.chromium.org/1249543003/diff/450001/src/gpu/GrGpu.cpp#newc...
src/gpu/GrGpu.cpp:165: if (width < 0 || height < 0) {
<= 0?

https://codereview.chromium.org/1249543003/diff/450001/src/gpu/GrGpu.cpp#newc...
src/gpu/GrGpu.cpp:350: if (width < 0 || height < 0) {
<= 0?

https://codereview.chromium.org/1249543003/diff/450001/src/gpu/GrSurface.cpp
File src/gpu/GrSurface.cpp (right):

https://codereview.chromium.org/1249543003/diff/450001/src/gpu/GrSurface.cpp#...
src/gpu/GrSurface.cpp:109: if (width < 0 || height < 0) {
<= 0?

https://codereview.chromium.org/1249543003/diff/450001/src/gpu/GrSurface.cpp#...
src/gpu/GrSurface.cpp:110: return nullptr;
false rather than nullptr

https://codereview.chromium.org/1249543003/diff/450001/src/gpu/GrSurface.cpp#...
src/gpu/GrSurface.cpp:115: SkMipMapLevel level(buffer, rowBytes, baseLevelWidth,
baseLevelHeight);
This doesn't copy the buffer, right?

https://codereview.chromium.org/1249543003/diff/450001/src/gpu/SkGr.cpp
File src/gpu/SkGr.cpp (right):

https://codereview.chromium.org/1249543003/diff/450001/src/gpu/SkGr.cpp#newco...
src/gpu/SkGr.cpp:565: static GrTexture* create_mipmapped_texture(GrContext* ctx,
You should coordinate with reed@ about landing this. I think you guys are headed
for merge conflict as he is restructuring a lot of the code in here.

https://codereview.chromium.org/1249543003/diff/450001/src/gpu/gl/GrGLGpu.cpp
File src/gpu/gl/GrGLGpu.cpp (right):

https://codereview.chromium.org/1249543003/diff/450001/src/gpu/gl/GrGLGpu.cpp...
src/gpu/gl/GrGLGpu.cpp:924: if (texelsCopy[currentMipLevel].fHeight >
std::numeric_limits<int>::max()
SK_MaxI32

https://codereview.chromium.org/1249543003/diff/450001/src/gpu/gl/GrGLGpu.cpp...
src/gpu/gl/GrGLGpu.cpp:925: || texelsCopy[currentMipLevel].fWidth >
std::numeric_limits<int>::max()) {
SK_MaxI32

https://codereview.chromium.org/1249543003/diff/450001/src/gpu/gl/GrGLGpu.cpp...
src/gpu/gl/GrGLGpu.cpp:1019: if (interface == nullptr) {
This can't happen, an interface is required to create a GrGLGpu.

https://codereview.chromium.org/1249543003/diff/450001/src/gpu/gl/GrGLGpu.cpp...
src/gpu/gl/GrGLGpu.cpp:1342: auto idDesc =
generate_and_bind_gl_texture(this->glInterface(), lifeCycle);
-auto

https://codereview.chromium.org/1249543003/diff/450001/src/gpu/gl/GrGLGpu.cpp...
src/gpu/gl/GrGLGpu.cpp:1350: auto initialTexParams =
set_initial_texture_params(this->glInterface());
-auto

[cblume, 2015-10-08 09:27:57]

I rebased to work along side reed@'s changes and incorporated the comments.

It does not build right now. I made a bad merge, presumably. I'll fix that soon.

https://codereview.chromium.org/1249543003/diff/450001/include/gpu/GrContext.h
File include/gpu/GrContext.h (right):

https://codereview.chromium.org/1249543003/diff/450001/include/gpu/GrContext....
include/gpu/GrContext.h:267: *                      element and it contains
nullptr fTexels, texture data is
On 2015/09/30 18:01:29, bsalomon wrote:
> Why one element and not just a 0 element array?

Oh. That...is a much better idea. I'll go through and fix these.

https://codereview.chromium.org/1249543003/diff/450001/src/gpu/GrContext.cpp
File src/gpu/GrContext.cpp (right):

https://codereview.chromium.org/1249543003/diff/450001/src/gpu/GrContext.cpp#...
src/gpu/GrContext.cpp:348: currentMipWidth = std::max(1, currentMipWidth / 2);
On 2015/09/30 18:01:29, bsalomon wrote:
> SkTMax

Noted. Although, I think my better option at this bit of code is to use the
height/width from the texelmap rather than compute it. (The SkMipMap code
generates mipmaps which aren't always half the size.)

https://codereview.chromium.org/1249543003/diff/450001/src/gpu/GrContext.cpp#...
src/gpu/GrContext.cpp:435: currentWidth = std::max(1, currentWidth / 2);
On 2015/09/30 18:01:29, bsalomon wrote:
> SkTMax

Done.

https://codereview.chromium.org/1249543003/diff/450001/src/gpu/GrContext.cpp#...
src/gpu/GrContext.cpp:436: currentHeight = std::max(1, currentHeight / 2);
On 2015/09/30 18:01:29, bsalomon wrote:
> SkTMax

Done.

https://codereview.chromium.org/1249543003/diff/450001/src/gpu/GrContext.cpp#...
src/gpu/GrContext.cpp:475: currentMipWidth = std::max(1, currentMipWidth / 2);
On 2015/09/30 18:01:29, bsalomon wrote:
> SkTMax

Done.

https://codereview.chromium.org/1249543003/diff/450001/src/gpu/GrContext.cpp#...
src/gpu/GrContext.cpp:476: currentMipHeight = std::max(1, currentMipHeight / 2);
On 2015/09/30 18:01:29, bsalomon wrote:
> SkTMax

Done.

https://codereview.chromium.org/1249543003/diff/450001/src/gpu/GrContext.cpp#...
src/gpu/GrContext.cpp:489: if (width < 0 || height < 0) {
On 2015/09/30 18:01:29, bsalomon wrote:
> Why not <=?

Done.

https://codereview.chromium.org/1249543003/diff/450001/src/gpu/GrGpu.cpp
File src/gpu/GrGpu.cpp (right):

https://codereview.chromium.org/1249543003/diff/450001/src/gpu/GrGpu.cpp#newc...
src/gpu/GrGpu.cpp:165: if (width < 0 || height < 0) {
On 2015/09/30 18:01:29, bsalomon wrote:
> <= 0?

Done.

https://codereview.chromium.org/1249543003/diff/450001/src/gpu/GrGpu.cpp#newc...
src/gpu/GrGpu.cpp:350: if (width < 0 || height < 0) {
On 2015/09/30 18:01:29, bsalomon wrote:
> <= 0?

Done.

https://codereview.chromium.org/1249543003/diff/450001/src/gpu/GrSurface.cpp
File src/gpu/GrSurface.cpp (right):

https://codereview.chromium.org/1249543003/diff/450001/src/gpu/GrSurface.cpp#...
src/gpu/GrSurface.cpp:109: if (width < 0 || height < 0) {
On 2015/09/30 18:01:29, bsalomon wrote:
> <= 0?

Done.

https://codereview.chromium.org/1249543003/diff/450001/src/gpu/GrSurface.cpp#...
src/gpu/GrSurface.cpp:110: return nullptr;
On 2015/09/30 18:01:29, bsalomon wrote:
> false rather than nullptr

Done.

https://codereview.chromium.org/1249543003/diff/450001/src/gpu/GrSurface.cpp#...
src/gpu/GrSurface.cpp:115: SkMipMapLevel level(buffer, rowBytes, baseLevelWidth,
baseLevelHeight);
On 2015/09/30 18:01:29, bsalomon wrote:
> This doesn't copy the buffer, right?

Correct. SkMipMapLevel only does a shallow copy of the pointer, not a deep copy
of the contents.

https://codereview.chromium.org/1249543003/diff/450001/src/gpu/SkGr.cpp
File src/gpu/SkGr.cpp (right):

https://codereview.chromium.org/1249543003/diff/450001/src/gpu/SkGr.cpp#newco...
src/gpu/SkGr.cpp:565: static GrTexture* create_mipmapped_texture(GrContext* ctx,
On 2015/09/30 18:01:29, bsalomon wrote:
> You should coordinate with reed@ about landing this. I think you guys are
headed
> for merge conflict as he is restructuring a lot of the code in here.

I rebased ontop of those changes. There was a conflict but I think it is
resolved.

https://codereview.chromium.org/1249543003/diff/450001/src/gpu/gl/GrGLGpu.cpp
File src/gpu/gl/GrGLGpu.cpp (right):

https://codereview.chromium.org/1249543003/diff/450001/src/gpu/gl/GrGLGpu.cpp...
src/gpu/gl/GrGLGpu.cpp:924: if (texelsCopy[currentMipLevel].fHeight >
std::numeric_limits<int>::max()
On 2015/09/30 18:01:29, bsalomon wrote:
> SK_MaxI32

Done.

https://codereview.chromium.org/1249543003/diff/450001/src/gpu/gl/GrGLGpu.cpp...
src/gpu/gl/GrGLGpu.cpp:925: || texelsCopy[currentMipLevel].fWidth >
std::numeric_limits<int>::max()) {
On 2015/09/30 18:01:29, bsalomon wrote:
> SK_MaxI32

Done.

https://codereview.chromium.org/1249543003/diff/450001/src/gpu/gl/GrGLGpu.cpp...
src/gpu/gl/GrGLGpu.cpp:1019: if (interface == nullptr) {
On 2015/09/30 18:01:29, bsalomon wrote:
> This can't happen, an interface is required to create a GrGLGpu.

Done.

https://codereview.chromium.org/1249543003/diff/450001/src/gpu/gl/GrGLGpu.cpp...
src/gpu/gl/GrGLGpu.cpp:1342: auto idDesc =
generate_and_bind_gl_texture(this->glInterface(), lifeCycle);
On 2015/09/30 18:01:29, bsalomon wrote:
> -auto

Done.

https://codereview.chromium.org/1249543003/diff/450001/src/gpu/gl/GrGLGpu.cpp...
src/gpu/gl/GrGLGpu.cpp:1350: auto initialTexParams =
set_initial_texture_params(this->glInterface());
On 2015/09/30 18:01:29, bsalomon wrote:
> -auto

Done.

[bsalomon, 2015-10-08 17:01:35]

https://codereview.chromium.org/1249543003/diff/450001/src/gpu/GrContext.cpp
File src/gpu/GrContext.cpp (right):

https://codereview.chromium.org/1249543003/diff/450001/src/gpu/GrContext.cpp#...
src/gpu/GrContext.cpp:348: currentMipWidth = std::max(1, currentMipWidth / 2);
On 2015/10/08 09:27:56, cblume wrote:
> On 2015/09/30 18:01:29, bsalomon wrote:
> > SkTMax
> 
> Noted. Although, I think my better option at this bit of code is to use the
> height/width from the texelmap rather than compute it. (The SkMipMap code
> generates mipmaps which aren't always half the size.)

Does that mean that SkMipMap and OpenGL will disagree about the dimensions of
the MIP levels? IIRC OpenGL is spec'ed to use the floor(original_width/2^i)
though implementations are allowed to relax it.

[cblume, 2015-10-08 18:33:10]

On 2015/10/08 17:01:35, bsalomon wrote:
> https://codereview.chromium.org/1249543003/diff/450001/src/gpu/GrContext.cpp
> File src/gpu/GrContext.cpp (right):
> 
>
https://codereview.chromium.org/1249543003/diff/450001/src/gpu/GrContext.cpp#...
> src/gpu/GrContext.cpp:348: currentMipWidth = std::max(1, currentMipWidth / 2);
> On 2015/10/08 09:27:56, cblume wrote:
> > On 2015/09/30 18:01:29, bsalomon wrote:
> > > SkTMax
> > 
> > Noted. Although, I think my better option at this bit of code is to use the
> > height/width from the texelmap rather than compute it. (The SkMipMap code
> > generates mipmaps which aren't always half the size.)
> 
> Does that mean that SkMipMap and OpenGL will disagree about the dimensions of
> the MIP levels? IIRC OpenGL is spec'ed to use the floor(original_width/2^i)
> though implementations are allowed to relax it.

That is a very good question and I am unsure. I started out with max(1,
previous_size / 2).
However, SkMipMap does not generate that. It generates sizes which are bumped a
tad
to just be near that. I suspect (but didn't investigate) that it is so it can
use its special
scaling function. Perhaps the special scaling function must work on images of a
certain size.

Once I learned SkMipMap behaved differently I began storing the sizes from what
was handed to me from SkMipMap and using that instead. It seemed to
work on my desktop and the bots we tried. But that isn't a guarantee it will
work
everywhere.

[cblume, 2015-10-08 18:34:16]

On 2015/10/08 18:33:10, cblume wrote:
> On 2015/10/08 17:01:35, bsalomon wrote:
> > https://codereview.chromium.org/1249543003/diff/450001/src/gpu/GrContext.cpp
> > File src/gpu/GrContext.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1249543003/diff/450001/src/gpu/GrContext.cpp#...
> > src/gpu/GrContext.cpp:348: currentMipWidth = std::max(1, currentMipWidth /
2);
> > On 2015/10/08 09:27:56, cblume wrote:
> > > On 2015/09/30 18:01:29, bsalomon wrote:
> > > > SkTMax
> > > 
> > > Noted. Although, I think my better option at this bit of code is to use
the
> > > height/width from the texelmap rather than compute it. (The SkMipMap code
> > > generates mipmaps which aren't always half the size.)
> > 
> > Does that mean that SkMipMap and OpenGL will disagree about the dimensions
of
> > the MIP levels? IIRC OpenGL is spec'ed to use the floor(original_width/2^i)
> > though implementations are allowed to relax it.
> 
> That is a very good question and I am unsure. I started out with max(1,
> previous_size / 2).
> However, SkMipMap does not generate that. It generates sizes which are bumped
a
> tad
> to just be near that. I suspect (but didn't investigate) that it is so it can
> use its special
> scaling function. Perhaps the special scaling function must work on images of
a
> certain size.
> 
> Once I learned SkMipMap behaved differently I began storing the sizes from
what
> was handed to me from SkMipMap and using that instead. It seemed to
> work on my desktop and the bots we tried. But that isn't a guarantee it will
> work
> everywhere.

Oh I should add that when creating each mipmap level OpenGL allows me to specify
the height and width. So rather than OpenGL guessing it is provided the size.

[cblume, 2015-10-08 20:44:51]

On 2015/10/08 18:34:16, cblume wrote:
> On 2015/10/08 18:33:10, cblume wrote:
> > On 2015/10/08 17:01:35, bsalomon wrote:
> > >
https://codereview.chromium.org/1249543003/diff/450001/src/gpu/GrContext.cpp
> > > File src/gpu/GrContext.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1249543003/diff/450001/src/gpu/GrContext.cpp#...
> > > src/gpu/GrContext.cpp:348: currentMipWidth = std::max(1, currentMipWidth /
> 2);
> > > On 2015/10/08 09:27:56, cblume wrote:
> > > > On 2015/09/30 18:01:29, bsalomon wrote:
> > > > > SkTMax
> > > > 
> > > > Noted. Although, I think my better option at this bit of code is to use
> the
> > > > height/width from the texelmap rather than compute it. (The SkMipMap
code
> > > > generates mipmaps which aren't always half the size.)
> > > 
> > > Does that mean that SkMipMap and OpenGL will disagree about the dimensions
> of
> > > the MIP levels? IIRC OpenGL is spec'ed to use the
floor(original_width/2^i)
> > > though implementations are allowed to relax it.
> > 
> > That is a very good question and I am unsure. I started out with max(1,
> > previous_size / 2).
> > However, SkMipMap does not generate that. It generates sizes which are
bumped
> a
> > tad
> > to just be near that. I suspect (but didn't investigate) that it is so it
can
> > use its special
> > scaling function. Perhaps the special scaling function must work on images
of
> a
> > certain size.
> > 
> > Once I learned SkMipMap behaved differently I began storing the sizes from
> what
> > was handed to me from SkMipMap and using that instead. It seemed to
> > work on my desktop and the bots we tried. But that isn't a guarantee it will
> > work
> > everywhere.
> 
> Oh I should add that when creating each mipmap level OpenGL allows me to
specify
> the height and width. So rather than OpenGL guessing it is provided the size.

I just checked the OpenGL spec along with aelias@. You are correct that the
mipmap sizes are max(1, floor(original_width / 2^i) ). SkMipMap does not do
that,
since it rounds numbers to make them even.

I was originally planning to use Skia's other scaling functions in a separate CL
so we got the quality we wanted. That would open up the possibility of
generating
correct sizes as well. I will instead put that in this CL since the patch should
respect the GL spec.

[cblume, 2015-10-13 16:05:01]

This should now respect the GL spec and use different stretching algorithms to
produce higher quality image results.

[bsalomon, 2015-10-19 14:56:18]

Hey Chris, started to look at this but it looks like the diff is a bit messed
up. It's showing several recent changes to Skia as part of the diff on
codereview.

https://codereview.chromium.org/1249543003/diff/650001/include/gpu/GrContext.h
File include/gpu/GrContext.h (right):

https://codereview.chromium.org/1249543003/diff/650001/include/gpu/GrContext....
include/gpu/GrContext.h:185: GrDrawContext* drawContext(GrRenderTarget* rt,
const SkSurfaceProps* surfaceProps = NULL) {
Is this supposed to be showing up in the diff? Looks like this is from a
different change that already landed.

[cblume, 2015-10-19 17:45:41]

On 2015/10/19 14:56:18, bsalomon wrote:
> Hey Chris, started to look at this but it looks like the diff is a bit messed
> up. It's showing several recent changes to Skia as part of the diff on
> codereview.
> 

Yeah. I guess perhaps I started this CL from the third party dir of my Chromium
repo, which may be why it is tracking Chromium's Skia instead of the unrolled
Skia repo?
When do you think the next roll will be? I can rebase after that if it is soon
to make things easier. That would perhaps be the easiest route. Otherwise, Patch
Set 29 is currently the easiest to compare. It is before a significant merge
conflict though, which should be reviewed post-merge conflict.

I also have these changes being tracked in a Skia repo (not in the third party
Chromium repo) if I should instead make a CL from there.


> https://codereview.chromium.org/1249543003/diff/650001/include/gpu/GrContext.h
> File include/gpu/GrContext.h (right):
> 
>
https://codereview.chromium.org/1249543003/diff/650001/include/gpu/GrContext....
> include/gpu/GrContext.h:185: GrDrawContext* drawContext(GrRenderTarget* rt,
> const SkSurfaceProps* surfaceProps = NULL) {
> Is this supposed to be showing up in the diff? Looks like this is from a
> different change that already landed.

It should not be part of my changes. I believe they have landed in Skia but not
in the Chromium rollup, hence why they showup here.

[cblume, 2015-10-27 06:11:28]

On 2015/10/19 17:45:41, cblume wrote:
> On 2015/10/19 14:56:18, bsalomon wrote:
> > Hey Chris, started to look at this but it looks like the diff is a bit
messed
> > up. It's showing several recent changes to Skia as part of the diff on
> > codereview.
> > 
> 
> Yeah. I guess perhaps I started this CL from the third party dir of my
Chromium
> repo, which may be why it is tracking Chromium's Skia instead of the unrolled
> Skia repo?
> When do you think the next roll will be? I can rebase after that if it is soon
> to make things easier. That would perhaps be the easiest route. Otherwise,
Patch
> Set 29 is currently the easiest to compare. It is before a significant merge
> conflict though, which should be reviewed post-merge conflict.
> 
> I also have these changes being tracked in a Skia repo (not in the third party
> Chromium repo) if I should instead make a CL from there.
> 
> 
> >
https://codereview.chromium.org/1249543003/diff/650001/include/gpu/GrContext.h
> > File include/gpu/GrContext.h (right):
> > 
> >
>
https://codereview.chromium.org/1249543003/diff/650001/include/gpu/GrContext....
> > include/gpu/GrContext.h:185: GrDrawContext* drawContext(GrRenderTarget* rt,
> > const SkSurfaceProps* surfaceProps = NULL) {
> > Is this supposed to be showing up in the diff? Looks like this is from a
> > different change that already landed.
> 
> It should not be part of my changes. I believe they have landed in Skia but
not
> in the Chromium rollup, hence why they showup here.

The patch set should now be easier to code review. Sorry it took so long to get
back to it.

[bsalomon, 2015-10-28 16:40:45]

I think I found some non-trivial wrinkles to this... see comments inline.

https://codereview.chromium.org/1249543003/diff/710001/src/core/SkMipMap.cpp
File src/core/SkMipMap.cpp (right):

https://codereview.chromium.org/1249543003/diff/710001/src/core/SkMipMap.cpp#...
src/core/SkMipMap.cpp:366: if (levelIndex > fCount) {
Is there a caller that relies on this clamping behavior? Wondering if we can
just fail here.

https://codereview.chromium.org/1249543003/diff/710001/src/gpu/GrContext.cpp
File src/gpu/GrContext.cpp (right):

https://codereview.chromium.org/1249543003/diff/710001/src/gpu/GrContext.cpp#...
src/gpu/GrContext.cpp:242: || texelsCopy[currentMipLevel].fHeight > SK_MaxS32) {
|| on prev line

Does SkMipMapLevel need to use uints for sizes? We generally use signed ints for
widths and heights and it seems like SkMipMapLevel should fall in line unless
there is a reason for it to disagree.

https://codereview.chromium.org/1249543003/diff/710001/src/gpu/GrContext.cpp#...
src/gpu/GrContext.cpp:367: drawContext->drawRect(GrClip::WideOpen(), paint,
matrix, rect, nullptr);
Seems like we will populate a mip chain on the temp texture and then do a draw
just to the base of the destination texture. I don't think the upper levels of
the dst will be updated.  Maybe we need a test to cover this.

Actually I take that back. The draw will dirty the upper levels on the dst,
causing us to call glGenerateMipMaps() which is what we're trying to avoid here.

I'm not exactly sure how to handle this without a lot more complication to this
change (binding each level of the dst as an fbo and drawing to them one by one).
Maybe this is the right long term approach but there would be a fair amount of
plumbing work to get that hooked up.

Perhaps GrContext::writeSurfacePixels shouldn't expose explicit mip level source
pixels for now? I don't think it affects the use case of sw-backed
SkBitmap/SkImages *if* we make GrTextureProvider always create a new texture
when the source data contains levels beyond 0 instead of the current behavior of
attempting to grab a scratch texture and calling writeSurfacePixels on it. Would
that work?

In light of comments I made below another approach would be to check whether the
caller gave us multiple levels. If so we only use a temp texture if the
drawPreference is kRequire (which should be rare) and document that in some
cases we will ignore the caller's upper levels and generate them in the 3D API.
(kRequire indicates that the underlying API [GL] doesn't support directly
uploading the level data to the texture because of API/extension restrictions so
we have to do some gymnastics by going through a temp, kPrefer indicates that
performance is better if we use a temp but it can be done without one).

https://codereview.chromium.org/1249543003/diff/710001/src/gpu/GrTexture.cpp
File src/gpu/GrTexture.cpp (right):

https://codereview.chromium.org/1249543003/diff/710001/src/gpu/GrTexture.cpp#...
src/gpu/GrTexture.cpp:29: this->didChangeGpuMemorySize();
Can we add a TODO and bug here that the desc and scratch key should be changed
here to (and file a bug)?

https://codereview.chromium.org/1249543003/diff/710001/src/gpu/GrTexture.cpp#...
src/gpu/GrTexture.cpp:96: if (desc.fIsMipMapped) {
Shouldn't fMipMapsStatus start as kValid *if* the caller gave data for all the
levels when creating the texture? Otherwise I think we'll call
glGenerateMipMaps() when mip map filtering is used with this texture.

https://codereview.chromium.org/1249543003/diff/710001/src/gpu/SkGr.cpp
File src/gpu/SkGr.cpp (right):

https://codereview.chromium.org/1249543003/diff/710001/src/gpu/SkGr.cpp#newco...
src/gpu/SkGr.cpp:65: builder[5] = isMipMapped;
This will cause us to potentially create two versions of the bitmap, mipped and
unmipped. Seems kinda wasteful. I see two potential fixes:

1) Just eat the glGenerateMipMaps cost if the first draw doesn't require MIPs
and a latter draw does.

2) When searching for an existing texture do something like this pseudocode

if (needMIPS) {
   GrMakeKeyFromImageID(&key, ..., true);
   if (tex = provider->findTexture(key)) {
      return tex;
   } else {
      tex = make_mipped_texture_and_assign_key(key, ...);
      // If we had a non-mipped version, let it go free
      GrMakeKeyFromImageID(&key, ..., false);
      nonMipTex = provider->findTexture(key);
      if (nonMipTex) {
          nonMipTex->removeUniqueKey();
      }
      return tex;
   }
} else {
   // prefer the mip version if it exists
   GrMakeKeyFromImageID(&key, ..., true);
   if (tex = provider->findTexture(key)) {
      return tex;
   }
   // check for non-mipped
   GrMakeKeyFromImageID(&key, ..., false);
   if (tex = provider->findTexture(key)) {
      return tex;
   }
   // create non-mipped
   tex = make_nonmipped_texture_and_assign_key(key, ...);
   return tex;
}

3) Don't include mip status in the key. If we find a texture using the key and
need mips and the texture doesn't already have mips, compute the upper levels
and add them to the texture. I think this is the best option. However, this
bumps into the problems I cited in GrContext::writeSurfacePixels. So it's
feasibility depends on how we answer that.

[cblume, 2015-10-28 22:49:40]

https://codereview.chromium.org/1249543003/diff/710001/src/core/SkMipMap.cpp
File src/core/SkMipMap.cpp (right):

https://codereview.chromium.org/1249543003/diff/710001/src/core/SkMipMap.cpp#...
src/core/SkMipMap.cpp:366: if (levelIndex > fCount) {
On 2015/10/28 16:40:44, bsalomon wrote:
> Is there a caller that relies on this clamping behavior? Wondering if we can
> just fail here.

There is no caller that relies on this. I was mimicking what I saw in
SkMipMap::extractLevel. But I like your idea of simply returning false.

https://codereview.chromium.org/1249543003/diff/710001/src/gpu/GrContext.cpp
File src/gpu/GrContext.cpp (right):

https://codereview.chromium.org/1249543003/diff/710001/src/gpu/GrContext.cpp#...
src/gpu/GrContext.cpp:242: || texelsCopy[currentMipLevel].fHeight > SK_MaxS32) {
On 2015/10/28 16:40:44, bsalomon wrote:
> || on prev line
> 
> Does SkMipMapLevel need to use uints for sizes? We generally use signed ints
for
> widths and heights and it seems like SkMipMapLevel should fall in line unless
> there is a reason for it to disagree.

Okay, I'll make them ints and remove the range checks.

https://codereview.chromium.org/1249543003/diff/710001/src/gpu/GrContext.cpp#...
src/gpu/GrContext.cpp:367: drawContext->drawRect(GrClip::WideOpen(), paint,
matrix, rect, nullptr);
On 2015/10/28 16:40:44, bsalomon wrote:
> Seems like we will populate a mip chain on the temp texture and then do a draw
> just to the base of the destination texture. I don't think the upper levels of
> the dst will be updated.  Maybe we need a test to cover this.
> 
> Actually I take that back. The draw will dirty the upper levels on the dst,
> causing us to call glGenerateMipMaps() which is what we're trying to avoid
here.
> 
> I'm not exactly sure how to handle this without a lot more complication to
this
> change (binding each level of the dst as an fbo and drawing to them one by
one).
> Maybe this is the right long term approach but there would be a fair amount of
> plumbing work to get that hooked up.
> 
> Perhaps GrContext::writeSurfacePixels shouldn't expose explicit mip level
source
> pixels for now? I don't think it affects the use case of sw-backed
> SkBitmap/SkImages *if* we make GrTextureProvider always create a new texture
> when the source data contains levels beyond 0 instead of the current behavior
of
> attempting to grab a scratch texture and calling writeSurfacePixels on it.
Would
> that work?
> 
> In light of comments I made below another approach would be to check whether
the
> caller gave us multiple levels. If so we only use a temp texture if the
> drawPreference is kRequire (which should be rare) and document that in some
> cases we will ignore the caller's upper levels and generate them in the 3D
API.
> (kRequire indicates that the underlying API [GL] doesn't support directly
> uploading the level data to the texture because of API/extension restrictions
so
> we have to do some gymnastics by going through a temp, kPrefer indicates that
> performance is better if we use a temp but it can be done without one).
> 

You are correct. In this case, it populates the mip chain on the temp texture
then renders to the base level of the render target.

IIRC we had talked about any time we have a render target destination we would
prefer to call glGenerateMipMaps to prevent reading the texture back out to the
CPU. I think we wanted to avoid the call to glGenerateMipMaps if we could but in
the case of a render target we couldn't. I think I like the idea of documenting
that mip levels other than 0 will be ignored if we are using the temp texture.

I am okay with making GrTextureProvider always create a new texture when there
are more than the base mip level provided. It is unlikely that there will be a
texture that can be reused in those cases anyway. All the extra work is for the
case when a mipmapped texture is reused, right? So if we feel that it is
unlikely to be reused we might save a lot of trouble.

[bsalomon, 2015-10-29 14:07:23]

On 2015/10/28 22:49:40, cblume wrote:
> https://codereview.chromium.org/1249543003/diff/710001/src/core/SkMipMap.cpp
> File src/core/SkMipMap.cpp (right):
> 
>
https://codereview.chromium.org/1249543003/diff/710001/src/core/SkMipMap.cpp#...
> src/core/SkMipMap.cpp:366: if (levelIndex > fCount) {
> On 2015/10/28 16:40:44, bsalomon wrote:
> > Is there a caller that relies on this clamping behavior? Wondering if we can
> > just fail here.
> 
> There is no caller that relies on this. I was mimicking what I saw in
> SkMipMap::extractLevel. But I like your idea of simply returning false.
> 
> https://codereview.chromium.org/1249543003/diff/710001/src/gpu/GrContext.cpp
> File src/gpu/GrContext.cpp (right):
> 
>
https://codereview.chromium.org/1249543003/diff/710001/src/gpu/GrContext.cpp#...
> src/gpu/GrContext.cpp:242: || texelsCopy[currentMipLevel].fHeight > SK_MaxS32)
{
> On 2015/10/28 16:40:44, bsalomon wrote:
> > || on prev line
> > 
> > Does SkMipMapLevel need to use uints for sizes? We generally use signed ints
> for
> > widths and heights and it seems like SkMipMapLevel should fall in line
unless
> > there is a reason for it to disagree.
> 
> Okay, I'll make them ints and remove the range checks.
> 
>
https://codereview.chromium.org/1249543003/diff/710001/src/gpu/GrContext.cpp#...
> src/gpu/GrContext.cpp:367: drawContext->drawRect(GrClip::WideOpen(), paint,
> matrix, rect, nullptr);
> On 2015/10/28 16:40:44, bsalomon wrote:
> > Seems like we will populate a mip chain on the temp texture and then do a
draw
> > just to the base of the destination texture. I don't think the upper levels
of
> > the dst will be updated.  Maybe we need a test to cover this.
> > 
> > Actually I take that back. The draw will dirty the upper levels on the dst,
> > causing us to call glGenerateMipMaps() which is what we're trying to avoid
> here.
> > 
> > I'm not exactly sure how to handle this without a lot more complication to
> this
> > change (binding each level of the dst as an fbo and drawing to them one by
> one).
> > Maybe this is the right long term approach but there would be a fair amount
of
> > plumbing work to get that hooked up.
> > 
> > Perhaps GrContext::writeSurfacePixels shouldn't expose explicit mip level
> source
> > pixels for now? I don't think it affects the use case of sw-backed
> > SkBitmap/SkImages *if* we make GrTextureProvider always create a new texture
> > when the source data contains levels beyond 0 instead of the current
behavior
> of
> > attempting to grab a scratch texture and calling writeSurfacePixels on it.
> Would
> > that work?
> > 
> > In light of comments I made below another approach would be to check whether
> the
> > caller gave us multiple levels. If so we only use a temp texture if the
> > drawPreference is kRequire (which should be rare) and document that in some
> > cases we will ignore the caller's upper levels and generate them in the 3D
> API.
> > (kRequire indicates that the underlying API [GL] doesn't support directly
> > uploading the level data to the texture because of API/extension
restrictions
> so
> > we have to do some gymnastics by going through a temp, kPrefer indicates
that
> > performance is better if we use a temp but it can be done without one).
> > 
> 
> You are correct. In this case, it populates the mip chain on the temp texture
> then renders to the base level of the render target.
> 
> IIRC we had talked about any time we have a render target destination we would
> prefer to call glGenerateMipMaps to prevent reading the texture back out to
the
> CPU. I think we wanted to avoid the call to glGenerateMipMaps if we could but
in
> the case of a render target we couldn't. I think I like the idea of
documenting
> that mip levels other than 0 will be ignored if we are using the temp texture.
> 

Right, when the content is generated by rendering I don't think it makes sense
to read back, gen mips, and upload them. But this is a case where we are
performing a rendering only in service of uploaded data from the CPU.


> I am okay with making GrTextureProvider always create a new texture when there
> are more than the base mip level provided. It is unlikely that there will be a
> texture that can be reused in those cases anyway. All the extra work is for
the
> case when a mipmapped texture is reused, right? So if we feel that it is
> unlikely to be reused we might save a lot of trouble.

I propose that we do the following as v1:

1) Make the texture provider not recycle a texture if the client provided
multiple MIP levels, so that it doesn't call GrContext::writeSurfacePixels, but
instead calls GrGpu::createTexture().

2) Not put the mip status in the bitmap unique key and just eat the the
glGenMipMaps call if the first draw of a bitmap doesn't use MIPs but a latter
one does. I expect this will be quite rare. 

3) Make GrContext::writeSurfacePixels not handle anything but the base level for
now. Because of 1) and 2) we wouldn't make use of it anyway and there is the
complication of temp textures that makes it difficult.

Then we could see what the reduction of glGenMipMaps looks like in Chromium and
decide how to iterate. If we need to we can figure out how to make
writeSurfacePixels use a temp per level and bind each dest level as an FBO in
order to draw to it. I'm hoping we don't need this in the short term as it goes
a bit against the grain of our current object model for render targets and
textures. We're planning to refactor that in a big way, but that wasn't slated
to happen until sometime next year. But if we have to we can find something
expedient for the short term.

[cblume, 2015-11-02 10:48:46]

On 2015/10/29 14:07:23, bsalomon wrote:
> On 2015/10/28 22:49:40, cblume wrote:
> > https://codereview.chromium.org/1249543003/diff/710001/src/core/SkMipMap.cpp
> > File src/core/SkMipMap.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1249543003/diff/710001/src/core/SkMipMap.cpp#...
> > src/core/SkMipMap.cpp:366: if (levelIndex > fCount) {
> > On 2015/10/28 16:40:44, bsalomon wrote:
> > > Is there a caller that relies on this clamping behavior? Wondering if we
can
> > > just fail here.
> > 
> > There is no caller that relies on this. I was mimicking what I saw in
> > SkMipMap::extractLevel. But I like your idea of simply returning false.
> > 
> > https://codereview.chromium.org/1249543003/diff/710001/src/gpu/GrContext.cpp
> > File src/gpu/GrContext.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1249543003/diff/710001/src/gpu/GrContext.cpp#...
> > src/gpu/GrContext.cpp:242: || texelsCopy[currentMipLevel].fHeight >
SK_MaxS32)
> {
> > On 2015/10/28 16:40:44, bsalomon wrote:
> > > || on prev line
> > > 
> > > Does SkMipMapLevel need to use uints for sizes? We generally use signed
ints
> > for
> > > widths and heights and it seems like SkMipMapLevel should fall in line
> unless
> > > there is a reason for it to disagree.
> > 
> > Okay, I'll make them ints and remove the range checks.
> > 
> >
>
https://codereview.chromium.org/1249543003/diff/710001/src/gpu/GrContext.cpp#...
> > src/gpu/GrContext.cpp:367: drawContext->drawRect(GrClip::WideOpen(), paint,
> > matrix, rect, nullptr);
> > On 2015/10/28 16:40:44, bsalomon wrote:
> > > Seems like we will populate a mip chain on the temp texture and then do a
> draw
> > > just to the base of the destination texture. I don't think the upper
levels
> of
> > > the dst will be updated.  Maybe we need a test to cover this.
> > > 
> > > Actually I take that back. The draw will dirty the upper levels on the
dst,
> > > causing us to call glGenerateMipMaps() which is what we're trying to avoid
> > here.
> > > 
> > > I'm not exactly sure how to handle this without a lot more complication to
> > this
> > > change (binding each level of the dst as an fbo and drawing to them one by
> > one).
> > > Maybe this is the right long term approach but there would be a fair
amount
> of
> > > plumbing work to get that hooked up.
> > > 
> > > Perhaps GrContext::writeSurfacePixels shouldn't expose explicit mip level
> > source
> > > pixels for now? I don't think it affects the use case of sw-backed
> > > SkBitmap/SkImages *if* we make GrTextureProvider always create a new
texture
> > > when the source data contains levels beyond 0 instead of the current
> behavior
> > of
> > > attempting to grab a scratch texture and calling writeSurfacePixels on it.
> > Would
> > > that work?
> > > 
> > > In light of comments I made below another approach would be to check
whether
> > the
> > > caller gave us multiple levels. If so we only use a temp texture if the
> > > drawPreference is kRequire (which should be rare) and document that in
some
> > > cases we will ignore the caller's upper levels and generate them in the 3D
> > API.
> > > (kRequire indicates that the underlying API [GL] doesn't support directly
> > > uploading the level data to the texture because of API/extension
> restrictions
> > so
> > > we have to do some gymnastics by going through a temp, kPrefer indicates
> that
> > > performance is better if we use a temp but it can be done without one).
> > > 
> > 
> > You are correct. In this case, it populates the mip chain on the temp
texture
> > then renders to the base level of the render target.
> > 
> > IIRC we had talked about any time we have a render target destination we
would
> > prefer to call glGenerateMipMaps to prevent reading the texture back out to
> the
> > CPU. I think we wanted to avoid the call to glGenerateMipMaps if we could
but
> in
> > the case of a render target we couldn't. I think I like the idea of
> documenting
> > that mip levels other than 0 will be ignored if we are using the temp
texture.
> > 
> 
> Right, when the content is generated by rendering I don't think it makes sense
> to read back, gen mips, and upload them. But this is a case where we are
> performing a rendering only in service of uploaded data from the CPU.
> 
> 
> > I am okay with making GrTextureProvider always create a new texture when
there
> > are more than the base mip level provided. It is unlikely that there will be
a
> > texture that can be reused in those cases anyway. All the extra work is for
> the
> > case when a mipmapped texture is reused, right? So if we feel that it is
> > unlikely to be reused we might save a lot of trouble.
> 
> I propose that we do the following as v1:
> 
> 1) Make the texture provider not recycle a texture if the client provided
> multiple MIP levels, so that it doesn't call GrContext::writeSurfacePixels,
but
> instead calls GrGpu::createTexture().
> 
> 2) Not put the mip status in the bitmap unique key and just eat the the
> glGenMipMaps call if the first draw of a bitmap doesn't use MIPs but a latter
> one does. I expect this will be quite rare. 
> 
> 3) Make GrContext::writeSurfacePixels not handle anything but the base level
for
> now. Because of 1) and 2) we wouldn't make use of it anyway and there is the
> complication of temp textures that makes it difficult.
> 
> Then we could see what the reduction of glGenMipMaps looks like in Chromium
and
> decide how to iterate. If we need to we can figure out how to make
> writeSurfacePixels use a temp per level and bind each dest level as an FBO in
> order to draw to it. I'm hoping we don't need this in the short term as it
goes
> a bit against the grain of our current object model for render targets and
> textures. We're planning to refactor that in a big way, but that wasn't slated
> to happen until sometime next year. But if we have to we can find something
> expedient for the short term.

I agree with what you are suggesting. Done.

[bsalomon, 2015-11-02 16:01:28]

looking good, small comments below.

https://chromiumcodereview.appspot.com/1249543003/diff/770001/src/gpu/GrSurfa...
File src/gpu/GrSurface.cpp (right):

https://chromiumcodereview.appspot.com/1249543003/diff/770001/src/gpu/GrSurfa...
src/gpu/GrSurface.cpp:97: uint32_t pixelOpsFlags) {
tiny nit, align with above

https://chromiumcodereview.appspot.com/1249543003/diff/770001/src/gpu/SkGr.cpp
File src/gpu/SkGr.cpp (right):

https://chromiumcodereview.appspot.com/1249543003/diff/770001/src/gpu/SkGr.cp...
src/gpu/SkGr.cpp:377: GrTexture* refOriginalTexture(GrContext* ctx, const
GrTextureParams& params) override {
Rather than passing the full params could be pass something like "bool
willBeMippedHint" or something like that?

https://chromiumcodereview.appspot.com/1249543003/diff/770001/src/gpu/SkGr.cp...
src/gpu/SkGr.cpp:390: SkASSERT(!tex);
How does this assert work? It seems like if we get here then texknown to be
uninitialized (because there is no original key) or nullptr (findAndRefTex...
failed to find a texture).

[cblume, 2015-11-03 02:10:22]

https://codereview.chromium.org/1249543003/diff/770001/src/gpu/GrSurface.cpp
File src/gpu/GrSurface.cpp (right):

https://codereview.chromium.org/1249543003/diff/770001/src/gpu/GrSurface.cpp#...
src/gpu/GrSurface.cpp:97: uint32_t pixelOpsFlags) {
On 2015/11/02 16:01:28, bsalomon wrote:
> tiny nit, align with above

Done.

https://codereview.chromium.org/1249543003/diff/770001/src/gpu/SkGr.cpp
File src/gpu/SkGr.cpp (right):

https://codereview.chromium.org/1249543003/diff/770001/src/gpu/SkGr.cpp#newco...
src/gpu/SkGr.cpp:377: GrTexture* refOriginalTexture(GrContext* ctx, const
GrTextureParams& params) override {
On 2015/11/02 16:01:28, bsalomon wrote:
> Rather than passing the full params could be pass something like "bool
> willBeMippedHint" or something like that?

Done.

https://codereview.chromium.org/1249543003/diff/770001/src/gpu/SkGr.cpp#newco...
src/gpu/SkGr.cpp:390: SkASSERT(!tex);
On 2015/11/02 16:01:28, bsalomon wrote:
> How does this assert work? It seems like if we get here then texknown to be
> uninitialized (because there is no original key) or nullptr (findAndRefTex...
> failed to find a texture).

I think this is a mistake on my part. This assert plus its comments were
originally part of create_mipmapped_texture(), which was called by
GrRefCachedBitmapTexture. You can see this in patch set 20. When the design
changed I moved the assert here but I think you are right -- it might not make
sense any more. I took it out.

[bsalomon, 2015-11-03 14:49:18]

lgtm

[cblume, 2015-11-09 21:26:32]

https://chromiumcodereview.appspot.com/1249543003/diff/710001/src/gpu/GrTextu...
File src/gpu/GrTexture.cpp (right):

https://chromiumcodereview.appspot.com/1249543003/diff/710001/src/gpu/GrTextu...
src/gpu/GrTexture.cpp:96: if (desc.fIsMipMapped) {
On 2015/10/28 16:40:44, bsalomon wrote:
> Shouldn't fMipMapsStatus start as kValid *if* the caller gave data for all the
> levels when creating the texture? Otherwise I think we'll call
> glGenerateMipMaps() when mip map filtering is used with this texture.

Done.

[cblume, 2015-11-09 22:43:02]

https://chromiumcodereview.appspot.com/1249543003/diff/710001/src/gpu/GrTextu...
File src/gpu/GrTexture.cpp (right):

https://chromiumcodereview.appspot.com/1249543003/diff/710001/src/gpu/GrTextu...
src/gpu/GrTexture.cpp:29: this->didChangeGpuMemorySize();
On 2015/10/28 16:40:44, bsalomon wrote:
> Can we add a TODO and bug here that the desc and scratch key should be changed
> here to (and file a bug)?

Done.

[cblume, 2015-12-04 14:25:17]

I have fixed a handful of bugs (memory corruption, compressed non-mipmapped
images being uploaded incorrectly) and cleaned up a few small details that were
bugging me (an assert stopped working but is now fixed, the size of the
SkAutoSMalloc is now typedefed so I am not repeating the #if #else #endif all
over).

I have run a before-vs-after dm --src gm and the only differences are minor
differences on the edges of mipmapped shapes (specifically, anisotropic_hq.png
and tiledscaledbitmap.png). The max color mismatch is 2 and 1, respectively.

So I believe this is more than acceptable for a rebaseline into gold. Can you
help me do that?

Thanks!

[bsalomon, 2015-12-04 16:43:42]

On 2015/12/04 14:25:17, cblume wrote:
> I have fixed a handful of bugs (memory corruption, compressed non-mipmapped
> images being uploaded incorrectly) and cleaned up a few small details that
were
> bugging me (an assert stopped working but is now fixed, the size of the
> SkAutoSMalloc is now typedefed so I am not repeating the #if #else #endif all
> over).
> 
> I have run a before-vs-after dm --src gm and the only differences are minor
> differences on the edges of mipmapped shapes (specifically, anisotropic_hq.png
> and tiledscaledbitmap.png). The max color mismatch is 2 and 1, respectively.
> 
> So I believe this is more than acceptable for a rebaseline into gold. Can you
> help me do that?
> 
> Thanks!

I started to triage them but a lot of them don't look correct. Here is the set
from patchset 60.

[bsalomon, 2015-12-04 16:44:04]

On 2015/12/04 14:25:17, cblume wrote:
> I have fixed a handful of bugs (memory corruption, compressed non-mipmapped
> images being uploaded incorrectly) and cleaned up a few small details that
were
> bugging me (an assert stopped working but is now fixed, the size of the
> SkAutoSMalloc is now typedefed so I am not repeating the #if #else #endif all
> over).
> 
> I have run a before-vs-after dm --src gm and the only differences are minor
> differences on the edges of mipmapped shapes (specifically, anisotropic_hq.png
> and tiledscaledbitmap.png). The max color mismatch is 2 and 1, respectively.
> 
> So I believe this is more than acceptable for a rebaseline into gold. Can you
> help me do that?
> 
> Thanks!

I started to triage them but a lot of them don't look correct. Here is the set
from patchset 60:
https://gold.skia.org/search2?issue=1249543003&unt=true&query=source_type%3Dg...

[cblume, 2015-12-04 18:47:54]

Well dang, that's unfortunate.

Looking at them, they seem to be maybe only one or two bugs since many images
have the same symptoms as others.

Is there a smart way to debug this? In order of my preferences:
1.) Do we have duplicate machines that I can attach a debugger to and walk
through? If not,
2.) Can I send quick-turnaround workloads to a specific bot like just running
this source in dm? If not,
3.) I can just make a bunch of patch sets and add try bots to them. These patch
set can printf information which I can grab from the bot's log. However, the
tests run across a thread pool so it won't be very clean. I can change the code
to force the thread pool to only have one thread but that would consume a lot
more bot time if I cannot target a single source in dm.

How do you suggest I approach this?

[cblume, 2015-12-04 23:25:07]

Also, are we 100% certain this link is only showing patchset 60?
https://gold.skia.org/search2?issue=1249543003&unt=true&query=source_type%3Dg...

I see that it is filtering to this issue, but I have several failed patchsets on
this issue which look similar to these problems. Those have been fixed.

For example, the anisotropic_hq picture
https://gold.skia.org/diff?test=anisotropic_hq&left=c944336f0aad59a9f39845c26...
is a problem I was working on earlier and had fixed.

I see how I can search Skia's GM site for my issue but I am unsure how to filter
by patchset.

Thanks!

[bsalomon, 2015-12-07 15:55:29]

On 2015/12/04 18:47:54, cblume wrote:
> Well dang, that's unfortunate.
> 
> Looking at them, they seem to be maybe only one or two bugs since many images
> have the same symptoms as others.
> 
> Is there a smart way to debug this? In order of my preferences:
> 1.) Do we have duplicate machines that I can attach a debugger to and walk
> through? If not,

I'd check with boronet@ whether it is possible to remote into the bot machines.
I'm not sure.

> 2.) Can I send quick-turnaround workloads to a specific bot like just running
> this source in dm? If not,

You could probably modify the flags in dmflags.py as part of your try job.

> 3.) I can just make a bunch of patch sets and add try bots to them. These
patch
> set can printf information which I can grab from the bot's log. However, the
> tests run across a thread pool so it won't be very clean. I can change the
code
> to force the thread pool to only have one thread but that would consume a lot
> more bot time if I cannot target a single source in dm.

GPU tests all run on on thread

> 
> How do you suggest I approach this?

The easiest way would be to find a machine/device locally that repros.
Otherwise, debugging by try bot seems like the best approach.

[bsalomon, 2015-12-07 15:57:27]

On 2015/12/04 23:25:07, cblume wrote:
> Also, are we 100% certain this link is only showing patchset 60?
>
https://gold.skia.org/search2?issue=1249543003&unt=true&query=source_type%3Dg...
> 
> I see that it is filtering to this issue, but I have several failed patchsets
on
> this issue which look similar to these problems. Those have been fixed.
> 
> For example, the anisotropic_hq picture
>
https://gold.skia.org/diff?test=anisotropic_hq&left=c944336f0aad59a9f39845c26...
> is a problem I was working on earlier and had fixed.
> 
> I see how I can search Skia's GM site for my issue but I am unsure how to
filter
> by patchset.
> 
> Thanks!

I believe they are only for patchset 60 unless there is a bug. You can see all
current try GM results here:

https://gold.skia.org/trybot

Clicking the patchset will show the images for it. If you think they're actually
not from the patchset that would be a bug in gold (stephana@ would be the
contact for such things).

[cblume, 2015-12-07 21:19:30]

On 2015/12/07 15:57:27, bsalomon wrote:
> Clicking the patchset will show the images for it. If you think they're
actually
> not from the patchset that would be a bug in gold (stephana@ would be the
> contact for such things).

Good news! I talked to stephana@ just to sanity check myself and make sure I
understood what was going on. He was extremely helpful and provided a lot of
clarity. If you go to https://gold.skia.org/trybot you'll see a column titled
"Results Up To" which for this issue has "Patchset 60". I asked if it was ONLY
Patchset 60 or all results up to Patchset 60. The way Stephana@ described it is
that it is all results but they get overwritten on a per-bot basis. So for
example, if I run both Windows and Mac trybots for patchset 59 but only Mac for
patchset 60 then that page will show Windows 59 and Mac 60.

What this means is it is possible that I ran it on a trybot earlier which I am
no longer including. Later today, I will go through and try to include all the
bots I had ever added so we'll have cleaner results. But a quick way to check
was to create a separate issue and put the latest code there. I did that with
https://codereview.chromium.org/1508663004/ and its results can be seen here
https://gold.skia.org/search2?issue=1508663004&unt=true&query=source_type%3Dg...

These results are much closer to what I expect. The major offenders have a line
drawn 1 pixel below, or some variation of the mipmap algorithms being different.
But the results look like what we expect as opposed to clear errors.

[bsalomon, 2015-12-07 21:38:53]

On 2015/12/07 21:19:30, cblume wrote:
> On 2015/12/07 15:57:27, bsalomon wrote:
> > Clicking the patchset will show the images for it. If you think they're
> actually
> > not from the patchset that would be a bug in gold (stephana@ would be the
> > contact for such things).
> 
> Good news! I talked to stephana@ just to sanity check myself and make sure I
> understood what was going on. He was extremely helpful and provided a lot of
> clarity. If you go to https://gold.skia.org/trybot you'll see a column titled
> "Results Up To" which for this issue has "Patchset 60". I asked if it was ONLY
> Patchset 60 or all results up to Patchset 60. The way Stephana@ described it
is
> that it is all results but they get overwritten on a per-bot basis. So for
> example, if I run both Windows and Mac trybots for patchset 59 but only Mac
for
> patchset 60 then that page will show Windows 59 and Mac 60.
> 
> What this means is it is possible that I ran it on a trybot earlier which I am
> no longer including. Later today, I will go through and try to include all the
> bots I had ever added so we'll have cleaner results. But a quick way to check
> was to create a separate issue and put the latest code there. I did that with
> https://codereview.chromium.org/1508663004/ and its results can be seen here
>
https://gold.skia.org/search2?issue=1508663004&unt=true&query=source_type%3Dg...
> 
> These results are much closer to what I expect. The major offenders have a
line
> drawn 1 pixel below, or some variation of the mipmap algorithms being
different.
> But the results look like what we expect as opposed to clear errors.

Great, I didn't realize that is how it worked. Are you able to triage the images
from your try bots? I can't remember what sort of permissions are needed to do
that.

[cblume, 2015-12-08 09:24:35]

I am about to go to bed. There is only one image which remains a bit out of
whack, and that is the iPad.
I'm rerunning the iPad test now hoping that it has passed when I wake up. If
not, I should investigate.

The other tests seem to be completely within acceptable range.

I just now realized that I should perhaps run this on all available hardware so
we can accept the new baseline on all the hardware.

[bsalomon, 2015-12-08 14:59:20]

On 2015/12/08 09:24:35, cblume wrote:
> I am about to go to bed. There is only one image which remains a bit out of
> whack, and that is the iPad.
> I'm rerunning the iPad test now hoping that it has passed when I wake up. If
> not, I should investigate.

Ok, I'll triage the others.

> 
> The other tests seem to be completely within acceptable range.
> 
> I just now realized that I should perhaps run this on all available hardware
so
> we can accept the new baseline on all the hardware.

It's up to you. It's ok to rebaseline them shortly after landing as well.

[cblume, 2016-01-07 18:23:31]

Description was changed from

==========
Creating functions for uploading a mipmapped texture.

BUG=476416
==========

to

==========
Creating functions for uploading a mipmapped texture.

BUG=476416
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[Chris Blume, 2016-01-07 21:42:23]

iOS is now fixed.
I did a lot of testing over at
https://codereview.chromium.org/1508663004/#ps1790001 and gathered information.
I then created an iOS OpenGL app to get a more simplified error reproduction.
It turned out to be iOS's limited non-power-of-two support with OpenGL ES 2.0.
It did not allow mipmaps on non-power-of-two textures.

Previously, the code path for texStorage was removed because it had not been
tested. I've been testing fairly thoroughly and decided to leave it in. It is
now an active code path with several months of testing behind it (including
finding bugs).

I believe this patch is now good to land and the slightly-different images are
okay for rebaselining.

[Chris Blume, 2016-01-07 21:42:50]

Description was changed from

==========
Creating functions for uploading a mipmapped texture.

BUG=476416
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
Creating functions for uploading a mipmapped texture.

BUG=476416
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[bsalomon, 2016-01-08 16:43:29]

This is understandably a very large change list. Could we make the
reintroduction of texstorage be a follow on CL?

https://codereview.chromium.org/1249543003/diff/1210001/src/gpu/GrTextureProv...
File src/gpu/GrTextureProvider.cpp (right):

https://codereview.chromium.org/1249543003/diff/1210001/src/gpu/GrTextureProv...
src/gpu/GrTextureProvider.cpp:35: if (texture->writePixels(0, 0, desc.fWidth,
desc.fHeight, desc.fConfig,
This works when writePixels uses an intermediate temporary texture that gets
rendered to texture?

[cblume, 2016-01-09 01:26:56]

On 2016/01/08 16:43:29, bsalomon wrote:
> This is understandably a very large change list. Could we make the
> reintroduction of texstorage be a follow on CL?

Good idea. I moved the glTexStorage over to
https://codereview.chromium.org/1570173004

Once this lands the changes will be very small over there (making it much easier
to see in isolation).

https://codereview.chromium.org/1249543003/diff/1210001/src/gpu/GrTextureProv...
File src/gpu/GrTextureProvider.cpp (right):

https://codereview.chromium.org/1249543003/diff/1210001/src/gpu/GrTextureProv...
src/gpu/GrTextureProvider.cpp:35: if (texture->writePixels(0, 0, desc.fWidth,
desc.fHeight, desc.fConfig,
On 2016/01/08 16:43:29, bsalomon wrote:
> This works when writePixels uses an intermediate temporary texture that gets
> rendered to texture?

I think I don't follow what you are asking.
This path is inside "if (texels.count() < 2) {".
This is the non-mipmap path.
So it should be exactly the same as before this patch.

It is missing the "!srcData ||" prior to writing the pixels. So if the texture
data is null, it would be different. However, inside writePixels we check if the
data is null as well:
https://codereview.chromium.org/1249543003/diff/1210001/src/gpu/GrGpu.cpp

So I believe this should work the way it always has.

Perhaps I should explicitly shortcut if there is no data just for clarity's sake
or because that decision is expected to be made at this level?

[bsalomon, 2016-01-11 17:16:38]

https://codereview.chromium.org/1249543003/diff/1210001/src/gpu/GrTextureProv...
File src/gpu/GrTextureProvider.cpp (right):

https://codereview.chromium.org/1249543003/diff/1210001/src/gpu/GrTextureProv...
src/gpu/GrTextureProvider.cpp:35: if (texture->writePixels(0, 0, desc.fWidth,
desc.fHeight, desc.fConfig,
On 2016/01/09 01:26:56, cblume wrote:
> On 2016/01/08 16:43:29, bsalomon wrote:
> > This works when writePixels uses an intermediate temporary texture that gets
> > rendered to texture?
> 
> I think I don't follow what you are asking.
> This path is inside "if (texels.count() < 2) {".
> This is the non-mipmap path.
> So it should be exactly the same as before this patch.
> 
> It is missing the "!srcData ||" prior to writing the pixels. So if the texture
> data is null, it would be different. However, inside writePixels we check if
the
> data is null as well:
> https://codereview.chromium.org/1249543003/diff/1210001/src/gpu/GrGpu.cpp
> 
> So I believe this should work the way it always has.
> 
> Perhaps I should explicitly shortcut if there is no data just for clarity's
sake
> or because that decision is expected to be made at this level?

Got it, my misunderstanding.

https://codereview.chromium.org/1249543003/diff/1230001/src/gpu/GrGpu.cpp
File src/gpu/GrGpu.cpp (right):

https://codereview.chromium.org/1249543003/diff/1230001/src/gpu/GrGpu.cpp#new...
src/gpu/GrGpu.cpp:134: if (!caps) {
This will never be null. This object is holding a ref to it.

https://codereview.chromium.org/1249543003/diff/1230001/src/gpu/GrTextureProv...
File src/gpu/GrTextureProvider.cpp (right):

https://codereview.chromium.org/1249543003/diff/1230001/src/gpu/GrTextureProv...
src/gpu/GrTextureProvider.cpp:53: SkTArray<SkMipMapLevel> texels(mipLevelCount);
Can avoid a heap allocation by making this SkSTArray<1, SkMipMapLevel> (probably
other similar places).

https://codereview.chromium.org/1249543003/diff/1230001/src/gpu/SkGr.cpp
File src/gpu/SkGr.cpp (right):

https://codereview.chromium.org/1249543003/diff/1230001/src/gpu/SkGr.cpp#newc...
src/gpu/SkGr.cpp:329: const uint32_t mipLevelCount = unsignedSignificantBits;
I believe everywhere this is passed takes an int not a uint.

https://codereview.chromium.org/1249543003/diff/1230001/src/gpu/gl/GrGLGpu.cpp
File src/gpu/gl/GrGLGpu.cpp (right):

https://codereview.chromium.org/1249543003/diff/1230001/src/gpu/gl/GrGLGpu.cp...
src/gpu/gl/GrGLGpu.cpp:772: GR_GL_CALL(&interface,
In GrGLGpu::bindTexture we respecify the other texture params when the client
may have messed with the texture params in OGL directly. Should we do that with
these as well?

https://codereview.chromium.org/1249543003/diff/1230001/src/gpu/gl/GrGLGpu.cp...
src/gpu/gl/GrGLGpu.cpp:891: || texelsShallowCopy[currentMipLevel].fWidth >
SK_MaxS32) {
style nit, || on previous line

[Chris Blume, 2016-01-12 12:49:32]

cblume@google.com changed reviewers:
+ cblume@google.com

[Chris Blume, 2016-01-12 12:49:34]

https://codereview.chromium.org/1249543003/diff/1230001/src/gpu/GrGpu.cpp
File src/gpu/GrGpu.cpp (right):

https://codereview.chromium.org/1249543003/diff/1230001/src/gpu/GrGpu.cpp#new...
src/gpu/GrGpu.cpp:134: if (!caps) {
On 2016/01/11 17:16:38, bsalomon wrote:
> This will never be null. This object is holding a ref to it.

Done.

https://codereview.chromium.org/1249543003/diff/1230001/src/gpu/GrTextureProv...
File src/gpu/GrTextureProvider.cpp (right):

https://codereview.chromium.org/1249543003/diff/1230001/src/gpu/GrTextureProv...
src/gpu/GrTextureProvider.cpp:53: SkTArray<SkMipMapLevel> texels(mipLevelCount);
On 2016/01/11 17:16:38, bsalomon wrote:
> Can avoid a heap allocation by making this SkSTArray<1, SkMipMapLevel>
(probably
> other similar places).

Done, including elsewhere in the CL.

https://codereview.chromium.org/1249543003/diff/1230001/src/gpu/SkGr.cpp
File src/gpu/SkGr.cpp (right):

https://codereview.chromium.org/1249543003/diff/1230001/src/gpu/SkGr.cpp#newc...
src/gpu/SkGr.cpp:329: const uint32_t mipLevelCount = unsignedSignificantBits;
On 2016/01/11 17:16:38, bsalomon wrote:
> I believe everywhere this is passed takes an int not a uint.

Done.

https://codereview.chromium.org/1249543003/diff/1230001/src/gpu/gl/GrGLGpu.cpp
File src/gpu/gl/GrGLGpu.cpp (right):

https://codereview.chromium.org/1249543003/diff/1230001/src/gpu/gl/GrGLGpu.cp...
src/gpu/gl/GrGLGpu.cpp:772: GR_GL_CALL(&interface,
On 2016/01/11 17:16:38, bsalomon wrote:
> In GrGLGpu::bindTexture we respecify the other texture params when the client
> may have messed with the texture params in OGL directly. Should we do that
with
> these as well?

That makes a lot of sense.

I began adding the max mipmap level to GrTextureParams but ran into a bit of
confusion. I'll submit that code later today.

https://codereview.chromium.org/1249543003/diff/1230001/src/gpu/gl/GrGLGpu.cp...
src/gpu/gl/GrGLGpu.cpp:891: || texelsShallowCopy[currentMipLevel].fWidth >
SK_MaxS32) {
On 2016/01/11 17:16:38, bsalomon wrote:
> style nit, || on previous line

Done.

[cblume, 2016-01-13 21:51:57]

The texture now stores the max mipmap level and sets it appropriately inside
GrGLGpu::bindTexture.

It tracks whether the max mipmap level was set. This is because we only want to
set it when we know that many levels actually exist.

If we call glGenerateMipmaps on iOS (which has limited non-power-of-two texture
support) with a NPOT texture it will return GL_INVALID_OPERATION if it cannot
generate the mipmaps (which we do not currently explicitly look for). Moreover,
the call to glGenerateMipmaps and glTexStorage2D may actually be setting the max
mipmap levels anyway.

[bsalomon, 2016-01-14 13:57:10]

On 2016/01/13 21:51:57, cblume wrote:
> The texture now stores the max mipmap level and sets it appropriately inside
> GrGLGpu::bindTexture.
> 
> It tracks whether the max mipmap level was set. This is because we only want
to
> set it when we know that many levels actually exist.
> 
> If we call glGenerateMipmaps on iOS (which has limited non-power-of-two
texture
> support) with a NPOT texture it will return GL_INVALID_OPERATION if it cannot
> generate the mipmaps (which we do not currently explicitly look for).

Are we calling GenerateMipMaps on ES2 contexts without GL_OES_texture_npot? If
so let's fix that first. We have chosen not to support MIP maps at all when
there are POT restrictions. So if we're calling glGenerateMipMaps on such
contexts, it's a bug.

> Moreover,
> the call to glGenerateMipmaps and glTexStorage2D may actually be setting the
max
> mipmap levels anyway.

I think in those cases we should record the predictable number of MIP maps they
create so that regardless of how they were computed the level count on the
texture is correct.

[bsalomon, 2016-01-14 13:57:23]

https://codereview.chromium.org/1249543003/diff/1270001/include/gpu/GrTexture.h
File include/gpu/GrTexture.h (right):

https://codereview.chromium.org/1249543003/diff/1270001/include/gpu/GrTexture...
include/gpu/GrTexture.h:65: int             fMaxMipMapLevel;
Could this be combined with the Desc's boolean? Maybe store a count on Desc and
know that 1 means "not mipmapped"?

https://codereview.chromium.org/1249543003/diff/1270001/src/gpu/SkGr.cpp
File src/gpu/SkGr.cpp (right):

https://codereview.chromium.org/1249543003/diff/1270001/src/gpu/SkGr.cpp#newc...
src/gpu/SkGr.cpp:370: canvas.drawBitmap(bitmap, SkIntToScalar(0),
SkIntToScalar(0), &paint);
Mike pointed out to me that this will generate MIP maps in the CPU backend in
order to create MIP maps for the GPU backend :)

I'm not sure Mike is still following the details on this Issue. Could you
contact him to figure out the best way to generate the SkMipMapLevels using the
cpu backend toolkit?

https://codereview.chromium.org/1249543003/diff/1270001/src/gpu/gl/GrGLGpu.cpp
File src/gpu/gl/GrGLGpu.cpp (right):

https://codereview.chromium.org/1249543003/diff/1270001/src/gpu/gl/GrGLGpu.cp...
src/gpu/gl/GrGLGpu.cpp:2725: GL_CALL(GenerateMipmap(target));
Should we set the mip count here, if not already set? Seems like the texture now
has a known number of MIP levels same as if we explicitly uploaded them

[Chris Blume, 2016-01-15 10:30:32]

On 2016/01/14 13:57:10, bsalomon wrote:
> On 2016/01/13 21:51:57, cblume wrote:
> > The texture now stores the max mipmap level and sets it appropriately inside
> > GrGLGpu::bindTexture.
> > 
> > It tracks whether the max mipmap level was set. This is because we only want
> to
> > set it when we know that many levels actually exist.
> > 
> > If we call glGenerateMipmaps on iOS (which has limited non-power-of-two
> texture
> > support) with a NPOT texture it will return GL_INVALID_OPERATION if it
cannot
> > generate the mipmaps (which we do not currently explicitly look for).
> 
> Are we calling GenerateMipMaps on ES2 contexts without GL_OES_texture_npot? If
> so let's fix that first. We have chosen not to support MIP maps at all when
> there are POT restrictions. So if we're calling glGenerateMipMaps on such
> contexts, it's a bug.

Sorry, I misspoke.
We are indeed only calling glGenerateMipmaps when POT is supported:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/skia/s...

> > Moreover,
> > the call to glGenerateMipmaps and glTexStorage2D may actually be setting the
> max
> > mipmap levels anyway.
> 
> I think in those cases we should record the predictable number of MIP maps
they
> create so that regardless of how they were computed the level count on the
> texture is correct.

I think you are right. That is a good idea.
The GL spec is clear that it will generate the full stack. And it is clear what
a full stack is.
So we can follow the spec and know how many levels there would be.

[Chris Blume, 2016-01-20 06:15:08]

https://codereview.chromium.org/1249543003/diff/1270001/include/gpu/GrTexture.h
File include/gpu/GrTexture.h (right):

https://codereview.chromium.org/1249543003/diff/1270001/include/gpu/GrTexture...
include/gpu/GrTexture.h:65: int             fMaxMipMapLevel;
On 2016/01/14 13:57:23, bsalomon wrote:
> Could this be combined with the Desc's boolean? Maybe store a count on Desc
and
> know that 1 means "not mipmapped"?

Done.

https://codereview.chromium.org/1249543003/diff/1270001/src/gpu/SkGr.cpp
File src/gpu/SkGr.cpp (right):

https://codereview.chromium.org/1249543003/diff/1270001/src/gpu/SkGr.cpp#newc...
src/gpu/SkGr.cpp:370: canvas.drawBitmap(bitmap, SkIntToScalar(0),
SkIntToScalar(0), &paint);
On 2016/01/14 13:57:23, bsalomon wrote:
> Mike pointed out to me that this will generate MIP maps in the CPU backend in
> order to create MIP maps for the GPU backend :)
> 
> I'm not sure Mike is still following the details on this Issue. Could you
> contact him to figure out the best way to generate the SkMipMapLevels using
the
> cpu backend toolkit?

I talked to Mike, asking for clarification on the CPU v. GPU issue. I added that
for Chrome, we specifically wanted to not use GPU (like how we were previously
calling glGenerateMipmap and blocking the critical GPU thread). Mike clarified
that (I believe, he can correct me) it was more to do with SkCanvas calls and
not using SkMipMap.

I had explained that we previously used SkMipMap but found two things: 1.) the
resulting image quality wasn't the quality we wanted, and 2.) the dimensions
didn't match what OpenGL required.

I believe #1 was actually because of a bug in my code. I had fixed the bug after
moving away from SkMipMap. So #1 might actually be okay.

So I reran dm --src gm with some SkMipMap code in place to see the dimensions
generated. And it turns out the dimensions are correct for what OpenGL would
want. I am unsure if 1.) I simply measured wrong originally or recalled
incorrectly, or 2.) the dimension issue has been fixed, or 3.) (the scary one) a
test which exposed bad dimensions may have been fixed to no longer expose the
problem. Right now, I'm leaning towards #1. Things seem good.

So I put the SkMipMap code back in. It is working well on my local machine, with
good quality images. We can look at Gold to see if the other platforms look good
as well. I believe they will.

https://codereview.chromium.org/1249543003/diff/1270001/src/gpu/gl/GrGLGpu.cpp
File src/gpu/gl/GrGLGpu.cpp (right):

https://codereview.chromium.org/1249543003/diff/1270001/src/gpu/gl/GrGLGpu.cp...
src/gpu/gl/GrGLGpu.cpp:2725: GL_CALL(GenerateMipmap(target));
On 2016/01/14 13:57:23, bsalomon wrote:
> Should we set the mip count here, if not already set? Seems like the texture
now
> has a known number of MIP levels same as if we explicitly uploaded them

Done.

[bsalomon, 2016-01-20 14:21:44]

I think the TSAN failures are known but ASAN indicates a leak. One comment
inline, otherwise looks good

https://codereview.chromium.org/1249543003/diff/1310001/src/gpu/gl/GrGLGpu.cpp
File src/gpu/gl/GrGLGpu.cpp (right):

https://codereview.chromium.org/1249543003/diff/1310001/src/gpu/gl/GrGLGpu.cp...
src/gpu/gl/GrGLGpu.cpp:2744: GL_CALL(TexParameteri(target,
GR_GL_TEXTURE_MIN_LOD, 0));
Should we be setting this on every draw? Maybe store the last set info for this
texture in GrGLTexture::TexParams so we don't redundantly specify it?

[cblume, 2016-01-21 23:08:50]

https://codereview.chromium.org/1249543003/diff/1310001/src/gpu/gl/GrGLGpu.cpp
File src/gpu/gl/GrGLGpu.cpp (right):

https://codereview.chromium.org/1249543003/diff/1310001/src/gpu/gl/GrGLGpu.cp...
src/gpu/gl/GrGLGpu.cpp:2744: GL_CALL(TexParameteri(target,
GR_GL_TEXTURE_MIN_LOD, 0));
On 2016/01/20 14:21:44, bsalomon wrote:
> Should we be setting this on every draw? Maybe store the last set info for
this
> texture in GrGLTexture::TexParams so we don't redundantly specify it?

I have updated this to now only call when the max mipmap level changes (or the
setAll flag is set). This matches better how the other calls to TexParameteri
work.

[bsalomon, 2016-01-22 14:33:36]

lgtm w/ tiny nit inline.

https://codereview.chromium.org/1249543003/diff/1350001/src/gpu/SkGr.cpp
File src/gpu/SkGr.cpp (right):

https://codereview.chromium.org/1249543003/diff/1350001/src/gpu/SkGr.cpp#newc...
src/gpu/SkGr.cpp:317: std::unique_ptr<SkMipMap> mipmaps(SkMipMap::Build(bitmap,
nullptr));
We don't currently use unique_ptr in Skia. That is likely to change in the
future, but for consistency please use SkAutoTDelete

[reed1, 2016-01-22 16:29:13]

In an effort to consolidate our public api, do we need a separate struct for the
mipmap levels? Can we not take SkPixmap?

https://codereview.chromium.org/1249543003/diff/1310001/src/core/SkMipMap.h
File src/core/SkMipMap.h (right):

https://codereview.chromium.org/1249543003/diff/1310001/src/core/SkMipMap.h#n...
src/core/SkMipMap.h:32: // getLevel() is 1-indexed because the SkMipMap contains
the generated
All of skia's other apis are zero-based for indices/loops. Lets use that
convention here as well.

int countLevels() const;
void getLevel(int index, Level*) const;

No need for the bool, since it is just an error to specify a bad index.

https://codereview.chromium.org/1249543003/diff/1350001/include/gpu/GrTexture...
File include/gpu/GrTextureProvider.h (right):

https://codereview.chromium.org/1249543003/diff/1350001/include/gpu/GrTexture...
include/gpu/GrTextureProvider.h:32: const SkTArray<SkMipMapLevel>& texels);
Why is this a TArray, instead of just [],count? In general we try to not require
our contains to be used for our public API.

[reed1, 2016-01-22 16:30:36]

This is a pretty extensive change. Have we been able to bench it to know what
sort of improvement it will bring?

[reed1, 2016-01-22 16:31:16]

reed@google.com changed reviewers:
- reed@chromium.org

[reed1, 2016-01-22 16:31:16]

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

[cblume, 2016-01-23 01:34:15]

On 2016/01/22 16:29:13, reed1 wrote:
> In an effort to consolidate our public api, do we need a separate struct for
the
> mipmap levels? Can we not take SkPixmap?

I agree about limiting external API.

I tried changing it to pass arrays of SkPixmap just now and ran into a problem.
https://codereview.chromium.org/1249543003/diff/1330001/src/gpu/gl/GrGLGpu.cpp
Line 688 on the right side.
To create the SkPixmap there I need a SkImageInfo.

I could create a function to get a SkImageInfo from a GrPixelConfig. (I think I
actually did this much earlier in this CL but it didn't map perfectly.) I think
this isn't really what we would want anyway.

More to the point, it would be very odd for various mipmap levels to have
different pixel configurations. It might make more sense for there to be 1 pixel
configuration that represents the entire mipmap stack.

I spoke to @bsalomon a bit and we thought about a very simple struct GrMipLevel
{ void* fPixel; size_t fRowBytes; }; inside perhaps GrTypes.h. (We would then
assume the width/height based on mip level and original width/height.)
I liked this idea and began coding it.
Then I realized it is very close to what this CL already has. We would be
introducing a new struct that is externally visible.

I am leaning towards consolidating SkMipMapLevel into GrMipLevel inside
GrTypes.h since getting the SkImageInfo from inside GrGLGpu::onTransferPixels
seems incorrect.

[Chris Blume, 2016-01-25 08:18:01]

https://codereview.chromium.org/1249543003/diff/1310001/src/core/SkMipMap.h
File src/core/SkMipMap.h (right):

https://codereview.chromium.org/1249543003/diff/1310001/src/core/SkMipMap.h#n...
src/core/SkMipMap.h:32: // getLevel() is 1-indexed because the SkMipMap contains
the generated
On 2016/01/22 16:29:13, reed1 wrote:
> All of skia's other apis are zero-based for indices/loops. Lets use that
> convention here as well.
> 
> int countLevels() const;
> void getLevel(int index, Level*) const;
> 
> No need for the bool, since it is just an error to specify a bad index.

Done.

https://codereview.chromium.org/1249543003/diff/1350001/include/gpu/GrTexture...
File include/gpu/GrTextureProvider.h (right):

https://codereview.chromium.org/1249543003/diff/1350001/include/gpu/GrTexture...
include/gpu/GrTextureProvider.h:32: const SkTArray<SkMipMapLevel>& texels);
On 2016/01/22 16:29:13, reed1 wrote:
> Why is this a TArray, instead of just [],count? In general we try to not
require
> our contains to be used for our public API.

Done.

[Chris Blume, 2016-01-25 08:19:53]

On 2016/01/22 16:30:36, reed1 wrote:
> This is a pretty extensive change. Have we been able to bench it to know what
> sort of improvement it will bring?

I will try to replicated these changes in my Chrome repo and run some tests.
We are hoping that it will unblock the GPU thread a tad during mipmap generation
(which is no longer happening on that thread).
So I should hopefully see small blips smoothed out.

I'll let you know once I have the results.

[reed1, 2016-01-25 14:08:34]

To help reduce the size, we might also look to land some of the "utility"
changes separately. e.g. I can land a version of your changes to skmipmap
(exposing a level iterate) separately.

[cblume, 2016-01-26 02:25:07]

On 2016/01/25 14:08:34, reed1 wrote:
> To help reduce the size, we might also look to land some of the "utility"
> changes separately. e.g. I can land a version of your changes to skmipmap
> (exposing a level iterate) separately.

Very good idea.

I have separated out as much as I could:

https://codereview.chromium.org/1631993002
This is just cleaning up trailing spaces and typos and other general formatting
that I crossed while I was working on this CL.

https://codereview.chromium.org/1636873002
This removes a rowBytes parameter that is passed through a bunch of functions
but never used.

https://codereview.chromium.org/1639693002
This adds direct access to SkMipMap's layers.

[cblume, 2016-02-24 07:59:24]

With the separate CLs pulled out, this has been cleaned up.
I just cherrypicked the changes over to my Chrome repo and will work on getting
the performance measurements to you today.

https://codereview.chromium.org/1249543003/diff/1350001/src/gpu/SkGr.cpp
File src/gpu/SkGr.cpp (right):

https://codereview.chromium.org/1249543003/diff/1350001/src/gpu/SkGr.cpp#newc...
src/gpu/SkGr.cpp:317: std::unique_ptr<SkMipMap> mipmaps(SkMipMap::Build(bitmap,
nullptr));
On 2016/01/22 14:33:36, bsalomon wrote:
> We don't currently use unique_ptr in Skia. That is likely to change in the
> future, but for consistency please use SkAutoTDelete

Done.

[reed1, 2016-02-24 12:18:44]

non-gpu side LGTM
deferring to brian for gpu

[bsalomon, 2016-02-24 14:43:15]

https://codereview.chromium.org/1249543003/diff/1450001/include/gpu/GrTexture.h
File include/gpu/GrTexture.h (right):

https://codereview.chromium.org/1249543003/diff/1450001/include/gpu/GrTexture...
include/gpu/GrTexture.h:49: GrTexture(GrGpu*, LifeCycle, const GrSurfaceDesc&,
bool wasMipMapDataProvided);
Do we need the old constructor? Or alternatively in the new cons is it true that
desc.fIsMipMapped == wasMipMapDataProvided for all callers?

https://codereview.chromium.org/1249543003/diff/1450001/src/gpu/gl/GrGLGpu.cpp
File src/gpu/gl/GrGLGpu.cpp (right):

https://codereview.chromium.org/1249543003/diff/1450001/src/gpu/gl/GrGLGpu.cp...
src/gpu/gl/GrGLGpu.cpp:1411: // We only support GL_TEXTURE_2D at the moment.
Can we change this to say we only *create* GL_TEXTURE_2D (since we now allow
clients to wrap EXTERNAL_OES and RECTANGLE) textures.

[cblume, 2016-02-24 18:19:19]

https://codereview.chromium.org/1249543003/diff/1450001/include/gpu/GrTexture.h
File include/gpu/GrTexture.h (right):

https://codereview.chromium.org/1249543003/diff/1450001/include/gpu/GrTexture...
include/gpu/GrTexture.h:49: GrTexture(GrGpu*, LifeCycle, const GrSurfaceDesc&,
bool wasMipMapDataProvided);
On 2016/02/24 14:43:15, bsalomon wrote:
> Do we need the old constructor? Or alternatively in the new cons is it true
that
> desc.fIsMipMapped == wasMipMapDataProvided for all callers?

Done.

https://codereview.chromium.org/1249543003/diff/1450001/src/gpu/gl/GrGLGpu.cpp
File src/gpu/gl/GrGLGpu.cpp (right):

https://codereview.chromium.org/1249543003/diff/1450001/src/gpu/gl/GrGLGpu.cp...
src/gpu/gl/GrGLGpu.cpp:1411: // We only support GL_TEXTURE_2D at the moment.
On 2016/02/24 14:43:15, bsalomon wrote:
> Can we change this to say we only *create* GL_TEXTURE_2D (since we now allow
> clients to wrap EXTERNAL_OES and RECTANGLE) textures.

Done.

[bsalomon, 2016-02-24 18:23:45]

lgtm

[cblume, 2016-02-26 18:37:22]

The CQ bit was checked by cblume@chromium.org

[cblume, 2016-02-26 18:37:23]

The patchset sent to the CQ was uploaded after l-g-t-m from reed@google.com,
bsalomon@google.com
Link to the patchset: https://codereview.chromium.org/1249543003/#ps1510001
(title: "Cleaning up spacing and comments.")

[commit-bot: I haz the power, 2016-02-26 18:37:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1249543003/1510001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1249543003/1510001

[commit-bot: I haz the power, 2016-02-26 18:38:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-26 18:38:54]

Try jobs failed on following builders:
  Build-Mac-Clang-Arm7-Debug-iOS-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Mac-Clang-Arm7...)
  Build-Mac-Clang-x86_64-Release-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Mac-Clang-x86_...)

[bsalomon, 2016-02-26 18:41:16]

You need to rebaseline on a recent change of mine. createTexture now takes a
SkBudgeted enum rather than a boolean for the budgeted param. Should be very
straightforward.

[cblume, 2016-02-26 18:42:26]

On 2016/02/26 18:41:16, bsalomon wrote:
> You need to rebaseline on a recent change of mine. createTexture now takes a
> SkBudgeted enum rather than a boolean for the budgeted param. Should be very
> straightforward.


Yup. Already on it. :) Forgot to rebaseline before checking that box.

[cblume, 2016-02-26 21:09:40]

The CQ bit was checked by cblume@chromium.org

[cblume, 2016-02-26 21:09:41]

The patchset sent to the CQ was uploaded after l-g-t-m from bsalomon@google.com,
reed@google.com
Link to the patchset: https://codereview.chromium.org/1249543003/#ps1530001
(title: "Rebasing.")

[commit-bot: I haz the power, 2016-02-26 21:09:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1249543003/1530001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1249543003/1530001

[commit-bot: I haz the power, 2016-02-26 21:20:51]

Description was changed from

==========
Creating functions for uploading a mipmapped texture.

BUG=476416
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
Creating functions for uploading a mipmapped texture.

BUG=476416
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...

Committed:
https://skia.googlesource.com/skia/+/55f2d2d57f4dd4109aa0c9dab6023373e3b928ec
==========

[commit-bot: I haz the power, 2016-02-26 21:20:54]

Committed patchset #78 (id:1530001) as
https://skia.googlesource.com/skia/+/55f2d2d57f4dd4109aa0c9dab6023373e3b928ec

[Kimmo Kinnunen, 2016-02-29 09:59:14]

You probably know this already, but this causes following errors in Chromium
with GPU rasterization:

[18413:18413:0229/113825:ERROR:gles2_cmd_decoder_autogen.h(2853)]
[.CommandBufferContext.RenderWorker-0x3d352c0d89a0.GpuRasterization]GL ERROR
:GL_INVALID_ENUM : glTexParameteri: pname was GL_TEXTURE_BASE_LEVEL
[18413:18413:0229/113825:ERROR:gles2_cmd_decoder_autogen.h(2853)]
[.CommandBufferContext.RenderWorker-0x3d352c0d89a0.GpuRasterization]GL ERROR
:GL_INVALID_ENUM : glTexParameteri: pname was GL_TEXTURE_MAX_LOD
[18413:18413:0229/113825:ERROR:gles2_cmd_decoder_autogen.h(2853)]
[.CommandBufferContext.RenderWorker-0x3d352c0d89a0.GpuRasterization]GL ERROR
:GL_INVALID_ENUM : glTexParameteri: pname was GL_TEXTURE_MAX_LEVEL_APPLE

(setting the texture params on ES2 context, where the params are supported only
in ES3)

[cblume, 2016-02-29 18:52:30]

On 2016/02/29 09:59:14, Kimmo Kinnunen wrote:
> You probably know this already, but this causes following errors in Chromium
> with GPU rasterization:
> 
> [18413:18413:0229/113825:ERROR:gles2_cmd_decoder_autogen.h(2853)]
> [.CommandBufferContext.RenderWorker-0x3d352c0d89a0.GpuRasterization]GL ERROR
> :GL_INVALID_ENUM : glTexParameteri: pname was GL_TEXTURE_BASE_LEVEL
> [18413:18413:0229/113825:ERROR:gles2_cmd_decoder_autogen.h(2853)]
> [.CommandBufferContext.RenderWorker-0x3d352c0d89a0.GpuRasterization]GL ERROR
> :GL_INVALID_ENUM : glTexParameteri: pname was GL_TEXTURE_MAX_LOD
> [18413:18413:0229/113825:ERROR:gles2_cmd_decoder_autogen.h(2853)]
> [.CommandBufferContext.RenderWorker-0x3d352c0d89a0.GpuRasterization]GL ERROR
> :GL_INVALID_ENUM : glTexParameteri: pname was GL_TEXTURE_MAX_LEVEL_APPLE
> 
> (setting the texture params on ES2 context, where the params are supported
only
> in ES3)

Good catch.
We talked a bit and created
https://bugs.chromium.org/p/chromium/issues/detail?id=590804 where we will fix
this.
Thanks for the sharp eyes!