Refactor Vulkan image, texture, RTs so that create and getter handles match.

Refactor Vulkan image, texture, RTs so that create and getter handles match.

This allows allows us to track and notify clients when either side changes
and image layout.

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

Committed: https://skia.googlesource.com/skia/+/b2df0c2702329be6380a943d548e7377a51d8565

[egdaniel, 2016-05-12 20:07:02]

Description was changed from

==========
Refactor Vulkan image, texture, RTs so that create and getter handles match.

This allows allows us to track and notify clients when either side changes
and image layout.

BUG=skia:
==========

to

==========
Refactor Vulkan image, texture, RTs so that create and getter handles match.

This allows allows us to track and notify clients when either side changes
and image layout.

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

[egdaniel, 2016-05-12 20:49:20]

egdaniel@google.com changed reviewers:
+ bsalomon@google.com, jvanverth@google.com

[egdaniel, 2016-05-12 20:49:21]

The main motivation for this change is to make sure the backend object that we
are using to create textures is the same as the one we return as a handle.
Additionally, with this change we provide support for us and a client to change
the image layout and notify the other.

I would start looking in GrVkImage for this change. The main difference is we
move a lot of state off of the Resource object and now the GrVkImage holds a
GrVkImageInfo struct (which is our backend object).

From there changes just branch out into all the subclass (Texture, RT, etc.)
mostly around the fact they can't create GrVkImage::Resources anymore and
instead need to create GrVkImageInfo's.

[bsalomon, 2016-05-13 13:25:48]

https://codereview.chromium.org/1974983002/diff/20001/src/gpu/vk/GrVkTexture.cpp
File src/gpu/vk/GrVkTexture.cpp (right):

https://codereview.chromium.org/1974983002/diff/20001/src/gpu/vk/GrVkTexture....
src/gpu/vk/GrVkTexture.cpp:24: bool /*isBorrowed*/)
unused constructor param?

[egdaniel, 2016-05-13 13:29:59]

https://codereview.chromium.org/1974983002/diff/20001/src/gpu/vk/GrVkTexture.cpp
File src/gpu/vk/GrVkTexture.cpp (right):

https://codereview.chromium.org/1974983002/diff/20001/src/gpu/vk/GrVkTexture....
src/gpu/vk/GrVkTexture.cpp:24: bool /*isBorrowed*/)
On 2016/05/13 13:25:48, bsalomon wrote:
> unused constructor param?

it is so this ctor and the next one can both be called by the templated Create
function. If we prefer to not do this I can pass an extra variable into the
Create function to tell it which ctor to use.

[bsalomon, 2016-05-13 14:29:50]

On 2016/05/13 13:29:59, egdaniel wrote:
>
https://codereview.chromium.org/1974983002/diff/20001/src/gpu/vk/GrVkTexture.cpp
> File src/gpu/vk/GrVkTexture.cpp (right):
> 
>
https://codereview.chromium.org/1974983002/diff/20001/src/gpu/vk/GrVkTexture....
> src/gpu/vk/GrVkTexture.cpp:24: bool /*isBorrowed*/)
> On 2016/05/13 13:25:48, bsalomon wrote:
> > unused constructor param?
> 
> it is so this ctor and the next one can both be called by the templated Create
> function. If we prefer to not do this I can pass an extra variable into the
> Create function to tell it which ctor to use.

That template function is pretty short and used in exactly two places as far as
I can tell. Adding weird constructors to preserve it seems unnecessary to me.

[egdaniel, 2016-05-13 15:06:53]

https://codereview.chromium.org/1974983002/diff/20001/src/gpu/vk/GrVkTexture.cpp
File src/gpu/vk/GrVkTexture.cpp (right):

https://codereview.chromium.org/1974983002/diff/20001/src/gpu/vk/GrVkTexture....
src/gpu/vk/GrVkTexture.cpp:24: bool /*isBorrowed*/)
On 2016/05/13 13:29:59, egdaniel wrote:
> On 2016/05/13 13:25:48, bsalomon wrote:
> > unused constructor param?
> 
> it is so this ctor and the next one can both be called by the templated Create
> function. If we prefer to not do this I can pass an extra variable into the
> Create function to tell it which ctor to use.

Done.

[bsalomon, 2016-05-13 15:11:07]

lgtm, but Jim should probably have a look as well.

[jvanverth1, 2016-05-13 16:14:41]

A few comments from me:

https://codereview.chromium.org/1974983002/diff/40001/include/gpu/vk/GrVkTypes.h
File include/gpu/vk/GrVkTypes.h (right):

https://codereview.chromium.org/1974983002/diff/40001/include/gpu/vk/GrVkType...
include/gpu/vk/GrVkTypes.h:42: void updateImageLayout(VkImageLayout layout) {
fImageLayout = layout; }
I don't understand how this would be used. The original contents of the
ImageInfo struct are copied from the client version to the wrapped texture, so
how do they pass this information down?

https://codereview.chromium.org/1974983002/diff/40001/src/gpu/vk/GrVkImage.cpp
File src/gpu/vk/GrVkImage.cpp (right):

https://codereview.chromium.org/1974983002/diff/40001/src/gpu/vk/GrVkImage.cp...
src/gpu/vk/GrVkImage.cpp:63: bool GrVkImage::GetImageInfo(const GrVkGpu* gpu,
const ImageDesc& imageDesc, GrVkImageInfo* info) {
'Get' seems like the wrong verb here -- it implies it already exists and is a
member of another class. Maybe 'Init'?

https://codereview.chromium.org/1974983002/diff/40001/src/gpu/vk/GrVkImage.h
File src/gpu/vk/GrVkImage.h (right):

https://codereview.chromium.org/1974983002/diff/40001/src/gpu/vk/GrVkImage.h#...
src/gpu/vk/GrVkImage.h:26: GrVkImage(const GrVkImageInfo& info, bool isBorrowed)
: fInfo(info), fIsBorrowed(isBorrowed) {
Suggestion: make isBorrowed an enum rather than a bool, so it's clear what false
and true mean.

https://codereview.chromium.org/1974983002/diff/40001/src/gpu/vk/GrVkRenderTa...
File src/gpu/vk/GrVkRenderTarget.cpp (right):

https://codereview.chromium.org/1974983002/diff/40001/src/gpu/vk/GrVkRenderTa...
src/gpu/vk/GrVkRenderTarget.cpp:197: VK_CALL(gpu, DestroyImage(gpu->device(),
info.fImage, nullptr));
This pattern is repeated in a number of places. Might be cleaner and easier to
maintain if these two lines were wrapped in a function.

https://codereview.chromium.org/1974983002/diff/40001/src/gpu/vk/GrVkRenderTa...
File src/gpu/vk/GrVkRenderTarget.h (right):

https://codereview.chromium.org/1974983002/diff/40001/src/gpu/vk/GrVkRenderTa...
src/gpu/vk/GrVkRenderTarget.h:131: GrVkImage*                 fMSAAImage;
Yeah, this makes so much more sense.

https://codereview.chromium.org/1974983002/diff/40001/src/gpu/vk/GrVkTextureR...
File src/gpu/vk/GrVkTextureRenderTarget.h (right):

https://codereview.chromium.org/1974983002/diff/40001/src/gpu/vk/GrVkTextureR...
src/gpu/vk/GrVkTextureRenderTarget.h:109: bool isWrapped,
Here's a case where using an enum for isBorrowed would help -- when calling
Create it's easy to get the two bools confused.

[egdaniel, 2016-05-13 18:00:05]

https://codereview.chromium.org/1974983002/diff/40001/include/gpu/vk/GrVkTypes.h
File include/gpu/vk/GrVkTypes.h (right):

https://codereview.chromium.org/1974983002/diff/40001/include/gpu/vk/GrVkType...
include/gpu/vk/GrVkTypes.h:42: void updateImageLayout(VkImageLayout layout) {
fImageLayout = layout; }
On 2016/05/13 16:14:40, jvanverth1 wrote:
> I don't understand how this would be used. The original contents of the
> ImageInfo struct are copied from the client version to the wrapped texture, so
> how do they pass this information down?

When they call getTextureHandle they will get a pointer to our internal
VkImageInfo. That is when we expect them to update the layout.

https://codereview.chromium.org/1974983002/diff/40001/src/gpu/vk/GrVkImage.cpp
File src/gpu/vk/GrVkImage.cpp (right):

https://codereview.chromium.org/1974983002/diff/40001/src/gpu/vk/GrVkImage.cp...
src/gpu/vk/GrVkImage.cpp:63: bool GrVkImage::GetImageInfo(const GrVkGpu* gpu,
const ImageDesc& imageDesc, GrVkImageInfo* info) {
On 2016/05/13 16:14:40, jvanverth1 wrote:
> 'Get' seems like the wrong verb here -- it implies it already exists and is a
> member of another class. Maybe 'Init'?

Done.

https://codereview.chromium.org/1974983002/diff/40001/src/gpu/vk/GrVkImage.h
File src/gpu/vk/GrVkImage.h (right):

https://codereview.chromium.org/1974983002/diff/40001/src/gpu/vk/GrVkImage.h#...
src/gpu/vk/GrVkImage.h:26: GrVkImage(const GrVkImageInfo& info, bool isBorrowed)
: fInfo(info), fIsBorrowed(isBorrowed) {
On 2016/05/13 16:14:40, jvanverth1 wrote:
> Suggestion: make isBorrowed an enum rather than a bool, so it's clear what
false
> and true mean.

Done.

https://codereview.chromium.org/1974983002/diff/40001/src/gpu/vk/GrVkRenderTa...
File src/gpu/vk/GrVkRenderTarget.cpp (right):

https://codereview.chromium.org/1974983002/diff/40001/src/gpu/vk/GrVkRenderTa...
src/gpu/vk/GrVkRenderTarget.cpp:197: VK_CALL(gpu, DestroyImage(gpu->device(),
info.fImage, nullptr));
On 2016/05/13 16:14:40, jvanverth1 wrote:
> This pattern is repeated in a number of places. Might be cleaner and easier to
> maintain if these two lines were wrapped in a function.

Done.

https://codereview.chromium.org/1974983002/diff/40001/src/gpu/vk/GrVkTextureR...
File src/gpu/vk/GrVkTextureRenderTarget.h (right):

https://codereview.chromium.org/1974983002/diff/40001/src/gpu/vk/GrVkTextureR...
src/gpu/vk/GrVkTextureRenderTarget.h:109: bool isWrapped,
On 2016/05/13 16:14:40, jvanverth1 wrote:
> Here's a case where using an enum for isBorrowed would help -- when calling
> Create it's easy to get the two bools confused.

Done.

[jvanverth1, 2016-05-13 18:04:23]

lgtm

[egdaniel, 2016-05-13 18:07:54]

The CQ bit was checked by egdaniel@google.com

[egdaniel, 2016-05-13 18:07:55]

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

[commit-bot: I haz the power, 2016-05-13 18:08:01]

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

[commit-bot: I haz the power, 2016-05-13 18:30:40]

Description was changed from

==========
Refactor Vulkan image, texture, RTs so that create and getter handles match.

This allows allows us to track and notify clients when either side changes
and image layout.

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

to

==========
Refactor Vulkan image, texture, RTs so that create and getter handles match.

This allows allows us to track and notify clients when either side changes
and image layout.

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

Committed:
https://skia.googlesource.com/skia/+/b2df0c2702329be6380a943d548e7377a51d8565
==========

[commit-bot: I haz the power, 2016-05-13 18:30:42]

Committed patchset #5 (id:80001) as
https://skia.googlesource.com/skia/+/b2df0c2702329be6380a943d548e7377a51d8565