Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2337803002:
Don't copy image buffer when calculating size.
DescriptionDon't copy image buffer when calculating size.
When we call ->refEncoded() we trigger a buffer copy, when all we really
needed was to see if the data existed.
BUG=646089
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2337803002
Patch Set 1 #
Total comments: 4
Patch Set 2 : Removing old comment, making code smaller. #Patch Set 3 : Finding out which test is failing. #Patch Set 4 : I think the cacherator test should be passing now? #Patch Set 5 : Removing debug info. #
Total comments: 2
Patch Set 6 : Add isEncoded to SkCacherator #Patch Set 7 : Using canGenerateTexture #Patch Set 8 : Adding 'override' #
Total comments: 2
Patch Set 9 : Inverting comparison. Updating tests to fail iff mipped. #Patch Set 10 : Updating tests. #
Total comments: 2
Patch Set 11 : Uploading what was sitting around. #Patch Set 12 : Cleaning debug comments and testing. #Patch Set 13 : Adding canGenerateTexture() override to SkImage_Gpu. #Patch Set 14 : When no CPU work is needed, returning 0 means test expectation is false (0 == false). #
Total comments: 4
Patch Set 15 : Making SkImage_Gpu always return false. #
Messages
Total messages: 91 (55 generated)
Description was changed from ========== Don't copy image buffer when calculating size. When we call ->refEncoded() we trigger a buffer copy, when all we really needed was to see if the data existed. BUG=646089 ========== to ========== Don't copy image buffer when calculating size. When we call ->refEncoded() we trigger a buffer copy, when all we really needed was to see if the data existed. BUG=646089 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2337803002 ==========
cblume@chromium.org changed reviewers: + bsalomon@google.com, vmpstr@chromium.org
PTAL
https://codereview.chromium.org/2337803002/diff/1/src/image/SkImage_Gpu.cpp File src/image/SkImage_Gpu.cpp (right): https://codereview.chromium.org/2337803002/diff/1/src/image/SkImage_Gpu.cpp#n... src/image/SkImage_Gpu.cpp:489: // In the future we will access the cacherator and get the exact data that we want to (e.g. nit: You should update the comment. https://codereview.chromium.org/2337803002/diff/1/src/image/SkImage_Gpu.cpp#n... src/image/SkImage_Gpu.cpp:491: SkImageCacherator* data(as_IB(this)->peekCacherator()); Can we just inline this in the if? I don't think we use data anywhere else.
https://codereview.chromium.org/2337803002/diff/1/src/image/SkImage_Gpu.cpp File src/image/SkImage_Gpu.cpp (right): https://codereview.chromium.org/2337803002/diff/1/src/image/SkImage_Gpu.cpp#n... src/image/SkImage_Gpu.cpp:489: // In the future we will access the cacherator and get the exact data that we want to (e.g. On 2016/09/12 23:35:27, vmpstr wrote: > nit: You should update the comment. Done. https://codereview.chromium.org/2337803002/diff/1/src/image/SkImage_Gpu.cpp#n... src/image/SkImage_Gpu.cpp:491: SkImageCacherator* data(as_IB(this)->peekCacherator()); On 2016/09/12 23:35:27, vmpstr wrote: > Can we just inline this in the if? I don't think we use data anywhere else. Done. This actually makes it read much more easily to me as well: if(!peekCache() && !peekPixels()) essentially.
non-owner lgtm
lgtm
The CQ bit was checked by bsalomon@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Test-Ubuntu-GCC-ShuttleA-GPU-GTX660-x86_64-Release-GN-Trybot on master.client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-ShuttleA-GPU...)
The CQ bit was checked by bsalomon@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Test-Ubuntu-GCC-ShuttleA-GPU-GTX660-x86_64-Release-GN-Trybot on master.client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-ShuttleA-GPU...)
The CQ bit was checked by cblume@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2337803002/diff/80001/tests/ImageTest.cpp File tests/ImageTest.cpp (right): https://codereview.chromium.org/2337803002/diff/80001/tests/ImageTest.cpp#new... tests/ImageTest.cpp:898: kNone_SkFilterQuality, 1, true }, I am not yet familiar with the cacherator but as I stepped through this code, everything seemed to make sense. Do we really expect this to be failing?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2337803002/diff/80001/tests/ImageTest.cpp File tests/ImageTest.cpp (right): https://codereview.chromium.org/2337803002/diff/80001/tests/ImageTest.cpp#new... tests/ImageTest.cpp:898: kNone_SkFilterQuality, 1, true }, On 2016/09/16 08:10:45, cblume wrote: > I am not yet familiar with the cacherator but as I stepped through this code, > everything seemed to make sense. > > Do we really expect this to be failing? We definitely don't want this one to succeed. It will sw-rasterize a picture during decode time instead of gpu-rasterizing it during normal rasterization. Perhaps we should add a query to SkImage_Base or the cacherator about whether the image is encoded (if there isn't one already).
On 2016/09/16 13:23:49, bsalomon wrote: > https://codereview.chromium.org/2337803002/diff/80001/tests/ImageTest.cpp > File tests/ImageTest.cpp (right): > > https://codereview.chromium.org/2337803002/diff/80001/tests/ImageTest.cpp#new... > tests/ImageTest.cpp:898: kNone_SkFilterQuality, 1, true }, > On 2016/09/16 08:10:45, cblume wrote: > > I am not yet familiar with the cacherator but as I stepped through this code, > > everything seemed to make sense. > > > > Do we really expect this to be failing? > > We definitely don't want this one to succeed. It will sw-rasterize a picture > during decode time instead of gpu-rasterizing it during normal rasterization. > > Perhaps we should add a query to SkImage_Base or the cacherator about whether > the image is encoded (if there isn't one already). I don't fully understand. Maybe I can describe my mental model of it and you can help me understand what I have wrong. The old code would exit early if the data was in encoded form and we couldn't peekPixels, which I think means we don't yet have a decoded copy. So as I understand it, we exited early to not cause a decode to happen. The new code checks instead if we can peek into the cacherator and pixels. I think I misunderstand the cacherator. I would have thought it was checking for a decoded copy or an entry into a cache, which I would have guessed was also decoded. The test which was previously failing but now passing seems to just create a picture from a recording. I imagine there is no encoded version of the data here. I also imagine it was failing because we couldn't peekPixels because it hadn't yet been rastered. It was still a recording, I guess? For that test to now pass, would that mean peeking into the cacherator is causing the recording to be rastered? Is that what you meant by it causing a sw-raster at decode time? If so, "peek"ing into the cacherator isn't really peeking, right?
The CQ bit was checked by cblume@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bsalomon@google.com, vmpstr@chromium.org Link to the patchset: https://codereview.chromium.org/2337803002/#ps80001 (title: "Removing debug info.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by cblume@chromium.org
On 2016/09/24 01:17:30, commit-bot: I haz the power wrote: > CQ is trying da patch. Follow status at > > https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or... Sorry. Cleaning up at the end of the week, hit the "Commit" checkbox and immediately remembered why I hadn't committed this yet. :) I unchecked it. bsalomon@ - Can you help me understand or point me in the right direction for the cacherator?
On 2016/09/24 06:50:26, cblume wrote: > On 2016/09/24 01:17:30, commit-bot: I haz the power wrote: > > CQ is trying da patch. Follow status at > > > > > https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or... > > Sorry. Cleaning up at the end of the week, hit the "Commit" checkbox and > immediately remembered why I hadn't committed this yet. :) I unchecked it. > > bsalomon@ - Can you help me understand or point me in the right direction for > the cacherator? What's the status of this review? I just saw another trace with 50ms spent in this code on the compositor thread :) Assuming the approach is good, I think we should land this.
On 2016/10/19 18:47:42, vmpstr wrote: > On 2016/09/24 06:50:26, cblume wrote: > > On 2016/09/24 01:17:30, commit-bot: I haz the power wrote: > > > CQ is trying da patch. Follow status at > > > > > > > > > https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or... > > > > Sorry. Cleaning up at the end of the week, hit the "Commit" checkbox and > > immediately remembered why I hadn't committed this yet. :) I unchecked it. > > > > bsalomon@ - Can you help me understand or point me in the right direction for > > the cacherator? > > What's the status of this review? I just saw another trace with 50ms spent in > this code on the compositor thread :) Assuming the approach is good, I think we > should land this. After talking with Mike I think what we should do is provide a direct query about whether an image is backed by a codec rather than using refEncodedData() or peekCacherator() as a proxy. (My assumption here is that getDeferredTextureImageData() should fail if the image is not codec backed). We probably don't want this in the public interface, however there is a private interface to SkImage called SkImage_Base. All SkImages are really SkImage_Bases and there is a helper as_IB(<image>) to convert a SkImage to a SkImage_Base. So, I'm proposing we add SkImage_Base::isEncoded() and use that in SkImage::getDeferredTextureImageData().
On 2016/10/19 21:36:40, bsalomon wrote: > On 2016/10/19 18:47:42, vmpstr wrote: > > On 2016/09/24 06:50:26, cblume wrote: > > > On 2016/09/24 01:17:30, commit-bot: I haz the power wrote: > > > > CQ is trying da patch. Follow status at > > > > > > > > > > > > > > https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or... > > > > > > Sorry. Cleaning up at the end of the week, hit the "Commit" checkbox and > > > immediately remembered why I hadn't committed this yet. :) I unchecked it. > > > > > > bsalomon@ - Can you help me understand or point me in the right direction > for > > > the cacherator? > > > > What's the status of this review? I just saw another trace with 50ms spent in > > this code on the compositor thread :) Assuming the approach is good, I think > we > > should land this. > > After talking with Mike I think what we should do is provide a direct query > about whether an image is backed by a codec rather than using refEncodedData() > or peekCacherator() as a proxy. (My assumption here is that > getDeferredTextureImageData() should fail if the image is not codec backed). We > probably don't want this in the public interface, however there is a private > interface to SkImage called SkImage_Base. All SkImages are really SkImage_Bases > and there is a helper as_IB(<image>) to convert a SkImage to a SkImage_Base. So, > I'm proposing we add SkImage_Base::isEncoded() and use that in > SkImage::getDeferredTextureImageData(). That sounds like a good idea. I much prefer direct and clear. I began working on this just now but I'm not yet familiar with the routing to get whether or not it is backed by an encoder. I see Chrome's DecodingImageGenerator. I guess from the SkImage_Base I can query the generator, which can override. What about outside Chrome? I haven't searched Android's code. Doesn't Skia also provide its own decoders which presumably could be used?
PTAL This is now the 3rd patch in a 3-patch set. The first is https://chromiumcodereview.appspot.com/2434323002/ The second is https://chromiumcodereview.appspot.com/2434243003/ Doing this in order should prevent anything from breaking. If any other platform uses getDeferredImage* we will want it to update its image generators to mark that they are backed by an encoded image. Similarly, if there are encoders inside Skia that can be accessed via SkImage_Base we'll want to update those as well.
The CQ bit was checked by cblume@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Rather than create isEncoded(), it was proposed we create canGenerateTexture(). That landed here: https://skia-review.googlesource.com/c/3780/ This patch set uses the new canGenerateTexture().
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Build-Mac-Clang-x86_64-Release-GN-Trybot on master.client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Mac-Clang-x86_...) Build-Ubuntu-Clang-x86_64-Debug-GN-Trybot on master.client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-Clang-x...)
The CQ bit was checked by cblume@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Test-Ubuntu-GCC-ShuttleA-GPU-GTX660-x86_64-Release-GN-Trybot on master.client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-ShuttleA-GPU...)
On 2016/10/23 09:24:29, cblume wrote: > Rather than create isEncoded(), it was proposed we create canGenerateTexture(). > That landed here: https://skia-review.googlesource.com/c/3780/ > > This patch set uses the new canGenerateTexture(). Hrmmmm failing the GPU bots. I think maybe I need to disable the other generators? Are all SkImage*Generators something like decoders?
reed@google.com changed reviewers: + fmalita@chromium.org, reed@google.com
https://codereview.chromium.org/2337803002/diff/140001/src/image/SkImage_Gpu.cpp File src/image/SkImage_Gpu.cpp (right): https://codereview.chromium.org/2337803002/diff/140001/src/image/SkImage_Gpu.... src/image/SkImage_Gpu.cpp:488: if (!as_IB(this)->canGenerateTexture(proxy)) { Is the predicate reversed here? If the image *can* generate a texture, than we should just return (nothing CPU expensive to cache). If it cannot, then we should proceed with extracting its pixels...
On 2016/10/24 13:42:00, reed1 wrote: > https://codereview.chromium.org/2337803002/diff/140001/src/image/SkImage_Gpu.cpp > File src/image/SkImage_Gpu.cpp (right): > > https://codereview.chromium.org/2337803002/diff/140001/src/image/SkImage_Gpu.... > src/image/SkImage_Gpu.cpp:488: if (!as_IB(this)->canGenerateTexture(proxy)) { > Is the predicate reversed here? If the image *can* generate a texture, than we > should just return (nothing CPU expensive to cache). If it cannot, then we > should proceed with extracting its pixels... Agreed, this looks backwards.Agreed, if it returns tr
On 2016/10/24 13:53:05, bsalomon wrote: > On 2016/10/24 13:42:00, reed1 wrote: > > > https://codereview.chromium.org/2337803002/diff/140001/src/image/SkImage_Gpu.cpp > > File src/image/SkImage_Gpu.cpp (right): > > > > > https://codereview.chromium.org/2337803002/diff/140001/src/image/SkImage_Gpu.... > > src/image/SkImage_Gpu.cpp:488: if (!as_IB(this)->canGenerateTexture(proxy)) { > > Is the predicate reversed here? If the image *can* generate a texture, than we > > should just return (nothing CPU expensive to cache). If it cannot, then we > > should proceed with extracting its pixels... > > Agreed, this looks backwards.Agreed, if it returns tr err.. accidentally hit send. I agree with Mike.
Mike points out that unit test for this function currently expects a cpu-memory, but not encoded SkImage to succeed. The new code with the test inverted would fail. It seems to me that it should succeed iff the drawing parameters require MIP levels. Otherwise, there isn't really anything to do other than an unnecessary memcpy.
The CQ bit was checked by cblume@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Test-Ubuntu-GCC-ShuttleA-GPU-GTX660-x86_64-Release-GN-Trybot on master.client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-ShuttleA-GPU...)
The CQ bit was checked by cblume@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Test-Ubuntu-GCC-ShuttleA-GPU-GTX660-x86_64-Release-GN-Trybot on master.client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-ShuttleA-GPU...)
https://codereview.chromium.org/2337803002/diff/140001/src/image/SkImage_Gpu.cpp File src/image/SkImage_Gpu.cpp (right): https://codereview.chromium.org/2337803002/diff/140001/src/image/SkImage_Gpu.... src/image/SkImage_Gpu.cpp:488: if (!as_IB(this)->canGenerateTexture(proxy)) { On 2016/10/24 13:42:00, reed1 wrote: > Is the predicate reversed here? If the image *can* generate a texture, than we > should just return (nothing CPU expensive to cache). If it cannot, then we > should proceed with extracting its pixels... Done. https://codereview.chromium.org/2337803002/diff/180001/tests/ImageTest.cpp File tests/ImageTest.cpp (right): https://codereview.chromium.org/2337803002/diff/180001/tests/ImageTest.cpp#ne... tests/ImageTest.cpp:922: sk_sp<SkImage> otherContextImage = create_gpu_image(otherContextInfo.grContext()); This test is now causing a errors on non-GPU: ../../tests/ImageTest.cpp:857 Expected image pixels to be the same. At 5,5 0xff000000 != 0xffffffff I'm looking into it now.
On 2016/10/25 02:51:01, cblume wrote: > https://codereview.chromium.org/2337803002/diff/140001/src/image/SkImage_Gpu.cpp > File src/image/SkImage_Gpu.cpp (right): > > https://codereview.chromium.org/2337803002/diff/140001/src/image/SkImage_Gpu.... > src/image/SkImage_Gpu.cpp:488: if (!as_IB(this)->canGenerateTexture(proxy)) { > On 2016/10/24 13:42:00, reed1 wrote: > > Is the predicate reversed here? If the image *can* generate a texture, than we > > should just return (nothing CPU expensive to cache). If it cannot, then we > > should proceed with extracting its pixels... > > Done. > > https://codereview.chromium.org/2337803002/diff/180001/tests/ImageTest.cpp > File tests/ImageTest.cpp (right): > > https://codereview.chromium.org/2337803002/diff/180001/tests/ImageTest.cpp#ne... > tests/ImageTest.cpp:922: sk_sp<SkImage> otherContextImage = > create_gpu_image(otherContextInfo.grContext()); > This test is now causing a errors on non-GPU: > ../../tests/ImageTest.cpp:857 Expected image pixels to be the same. At 5,5 > 0xff000000 != 0xffffffff > I'm looking into it now. There are many other passing tests which call create_gpu_image(). So this must have something to do with the extra context. If you look at the test before the /////////////// you will see it is now set to expected to pass. It used to be an expected failure. Should we mark another SkImageGenerator as expected to not generate textures easily when it is across contexts? And even if I expect this test to fail, it is still trying to compare pixels which is asserting. Any ideas?
cblume@chromium.org changed reviewers: + ericrk@chromium.org
+Eric. I'm not sure why I didn't add him from the get go.
non-owner LGTM
https://codereview.chromium.org/2337803002/diff/180001/tests/ImageTest.cpp File tests/ImageTest.cpp (right): https://codereview.chromium.org/2337803002/diff/180001/tests/ImageTest.cpp#ne... tests/ImageTest.cpp:922: sk_sp<SkImage> otherContextImage = create_gpu_image(otherContextInfo.grContext()); On 2016/10/25 02:51:01, cblume wrote: > This test is now causing a errors on non-GPU: > ../../tests/ImageTest.cpp:857 Expected image pixels to be the same. At 5,5 > 0xff000000 != 0xffffffff > I'm looking into it now. I think this one should still be false, right? If we call SkImage::getDeferredTextureImageData on an image that has a texture in Context A with a proxy for Context B it seems simpler to fail rather than to try to do a readback. This doesn't seem like a likely scenario for Chrome. If we are doing a readback with the new code (not sure why that'd be) and it is failing then my guess is that when we do the readback Context A isn't current so we're trying to readback a texture ID from Context A in Context B.
message: On 2016/10/25 18:27:15, bsalomon wrote: > I think this one should still be false, right? Are you asking if it should be false in practice or in theory? In practice, it is now true (hence why I changed it). In theory, I think the concept is supposed to be "Is this cheap to create?". In that case, I do not think a readback is cheap. Is there a way we can also mark readbacks the way we marked SkPictureImageGenerator over in https://skia-review.googlesource.com/c/3780/ ? > If we call > SkImage::getDeferredTextureImageData on an image that has a texture in Context A > with a proxy for Context B it seems simpler to fail rather than to try to do a > readback. This doesn't seem like a likely scenario for Chrome. If we are doing a > readback with the new code (not sure why that'd be) and it is failing then my > guess is that when we do the readback Context A isn't current so we're trying to > readback a texture ID from Context A in Context B. Because the pixels aren't the same, I think it is failing. Even if I mark the test as expected to fail, it still tries to compare the pixels (and fails trybots on that). Should I change the tests around so they aren't going to compare pixels if it is expected to fail?
On 2016/10/25 21:52:06, cblume wrote: > message: On 2016/10/25 18:27:15, bsalomon wrote: > > I think this one should still be false, right? > > Are you asking if it should be false in practice or in theory? > In practice, it is now true (hence why I changed it). > In theory, I think the concept is supposed to be "Is this cheap to create?". In > that case, I do not think a readback is cheap. Is there a way we can also mark > readbacks the way we marked SkPictureImageGenerator over in > https://skia-review.googlesource.com/c/3780/ ? Right, I meant that it should fail in theory. I don't really understand why it is succeeding in practice. I wouldn't think this image would have a generator at all. Should there be an override for bool canGenerateTexture(const GrContextThreadSafeProxy& proxy) const override in SkImage_Gpu? I think it should return true if the context represents the same context as the texture in the image. Perhaps we need a check in SkImage::getDeferredTextureImageData() that says if peekTexture() is not nullptr return false. > > > If we call > > SkImage::getDeferredTextureImageData on an image that has a texture in Context > A > > with a proxy for Context B it seems simpler to fail rather than to try to do a > > readback. This doesn't seem like a likely scenario for Chrome. If we are doing > a > > readback with the new code (not sure why that'd be) and it is failing then my > > guess is that when we do the readback Context A isn't current so we're trying > to > > readback a texture ID from Context A in Context B. > > Because the pixels aren't the same, I think it is failing. Even if I mark the > test as expected to fail, it still tries to compare the pixels (and fails > trybots on that). > Should I change the tests around so they aren't going to compare pixels if it is > expected to fail?
The CQ bit was checked by cblume@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Test-Ubuntu-GCC-ShuttleA-GPU-GTX660-x86_64-Release-GN-Trybot on master.client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-ShuttleA-GPU...)
The CQ bit was checked by cblume@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/10/24 14:07:02, bsalomon wrote: > Mike points out that unit test for this function currently expects a cpu-memory, > but not encoded SkImage to succeed. The new code with the test inverted would > fail. It seems to me that it should succeed iff the drawing parameters require > MIP levels. Otherwise, there isn't really anything to do other than an > unnecessary memcpy. I completely missed this message. Are you saying the tests that currently pass are expected to be both cpu-memory and non-encoded? That sounds right, since the tests had expected failure on the GPU cases. The last problem I was dealing with is I would mark a GPU test as expected to fail but inside the test it would assert that pixels didn't match between two GPU contexts. When I am passing the proxy around to see if we can get a texture of it, is there a way to know "this proxy belongs to a different context, so I should say there is no texture"?? Also, I don't fully follow the idea that only MIP-level generating params should succeed. Is that because of this comment? // We never want to scale at higher than SW medium quality, as SW medium matches GPU high.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Test-Ubuntu-GCC-ShuttleA-GPU-GTX660-x86_64-Release-GN-Trybot on master.client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-ShuttleA-GPU...)
On 2016/11/14 02:31:53, cblume wrote: > On 2016/10/24 14:07:02, bsalomon wrote: > > Mike points out that unit test for this function currently expects a > cpu-memory, > > but not encoded SkImage to succeed. The new code with the test inverted would > > fail. It seems to me that it should succeed iff the drawing parameters require > > MIP levels. Otherwise, there isn't really anything to do other than an > > unnecessary memcpy. > > I completely missed this message. > > Are you saying the tests that currently pass are expected to be both cpu-memory > and non-encoded? > That sounds right, since the tests had expected failure on the GPU cases. > > The last problem I was dealing with is I would mark a GPU test as expected to > fail but inside the test it would assert that pixels didn't match between two > GPU contexts. > > When I am passing the proxy around to see if we can get a texture of it, is > there a way to know "this proxy belongs to a different context, so I should say > there is no texture"?? The proxy has a context unique ID. If the image has a GrTexture then GrTexture::getContext() can be used to get its context and its context's unique ID can be compared to the unique ID on the proxy. > > > Also, I don't fully follow the idea that only MIP-level generating params should > succeed. Is that because of this comment? > // We never want to scale at higher than SW medium quality, as SW medium matches > GPU high. I think that it should always succeed for encoded SkImages regardless of MIPness. For SkImages that are backed by a decoded block of pixels I was suggesting that it should only succeed if we require MIP maps (since otherwise there is no real CPU work to do). However, upon further reflection this would defeat Chrome's desire to explicitly manage the texture lifetime rather than use Skia's cache. So I suppose it should succeed whenever to image is backed by cpu pixels, either encoded or decoded.
The CQ bit was checked by cblume@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Test-Ubuntu-GCC-ShuttleA-GPU-GTX660-x86_64-Release-GN-Trybot on master.client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-ShuttleA-GPU...)
The CQ bit was checked by cblume@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Test-Ubuntu-GCC-ShuttleA-GPU-GTX660-x86_64-Release-GN-Trybot on master.client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-ShuttleA-GPU...)
This is currently erroring because two textures have different pixel values. The image obtained from getDeferredTextureImageData is returning white, which is what we expect. The image that is failing is created from: SkAutoPixmapStorage scaled; scaled.alloc(scaled_info); image->scalePixels(scaled, testCase.fExpectedQuality); sk_sp<SkImage> scaledImage = SkImage::MakeRasterCopy(scaled); The variable 'image' is the one created from getDeferredTextureImageData in a separate context. Do we expect to be able to get a scaled image via readback this way? I need some help on this part. https://codereview.chromium.org/2337803002/diff/260001/include/gpu/GrContext.h File include/gpu/GrContext.h (right): https://codereview.chromium.org/2337803002/diff/260001/include/gpu/GrContext.... include/gpu/GrContext.h:490: uint32_t uniqueID() const { return fContextUniqueID; } I saw a few potential options here. Below, you'll see a few friend classes. I could add SkImage_Gpu as a friend class. But I figured adding this method would be a little nicer. The down side to adding this method is it is a new public API. So I wanted to call attention to it. https://codereview.chromium.org/2337803002/diff/260001/src/core/SkImageCacher... File src/core/SkImageCacherator.cpp (right): https://codereview.chromium.org/2337803002/diff/260001/src/core/SkImageCacher... src/core/SkImageCacherator.cpp:166: I'll remove this second blank line. Not sure how that got in there. https://codereview.chromium.org/2337803002/diff/260001/src/image/SkImage_Gpu.h File src/image/SkImage_Gpu.h (right): https://codereview.chromium.org/2337803002/diff/260001/src/image/SkImage_Gpu.... src/image/SkImage_Gpu.h:61: #endif This might be a tad large for being in the .h -- should I move it to the .cpp? I would then be adding conditional compilation to the .cpp, which I prefer not to do. https://codereview.chromium.org/2337803002/diff/260001/tests/ImageTest.cpp File tests/ImageTest.cpp (right): https://codereview.chromium.org/2337803002/diff/260001/tests/ImageTest.cpp#ne... tests/ImageTest.cpp:923: kNone_SkFilterQuality, 1, true }, The true/false here is a bit unclear. If it is cheap to create a texture (and thus, we don't need to allocate memory to create one), we return 0 size required to be allocated. 0 == false. So the scenario where it is cheap to create means an expected outcome of false. It doesn't really mean the test failed. This particular line is for multiple contexts, where creating a texture is not cheap.
On 2016/11/16 01:07:16, cblume wrote: > This is currently erroring because two textures have different pixel values. > > The image obtained from getDeferredTextureImageData is returning white, which is > what we expect. > > The image that is failing is created from: > > SkAutoPixmapStorage scaled; > scaled.alloc(scaled_info); > image->scalePixels(scaled, testCase.fExpectedQuality); > sk_sp<SkImage> scaledImage = SkImage::MakeRasterCopy(scaled); > > The variable 'image' is the one created from getDeferredTextureImageData in a > separate context. > > Do we expect to be able to get a scaled image via readback this way? I need some > help on this part. Gentle ping, I need help on this. I am unsure why this would be effected.
On 2016/11/17 21:42:20, cblume wrote: > On 2016/11/16 01:07:16, cblume wrote: > > This is currently erroring because two textures have different pixel values. > > > > The image obtained from getDeferredTextureImageData is returning white, which > is > > what we expect. > > > > The image that is failing is created from: > > > > SkAutoPixmapStorage scaled; > > scaled.alloc(scaled_info); > > image->scalePixels(scaled, testCase.fExpectedQuality); > > sk_sp<SkImage> scaledImage = SkImage::MakeRasterCopy(scaled); > > > > The variable 'image' is the one created from getDeferredTextureImageData in a > > separate context. > > > > Do we expect to be able to get a scaled image via readback this way? I need > some > > help on this part. > > Gentle ping, > I need help on this. I am unsure why this would be effected. Not sure - from looking through the code, it looks like there are a lot of points / cases where readback from the texture can fail. I don't see anything specific to scalePixels that would change things vs any other readback. Not sure if the fact that we're creating the texture in a different context causes readback problems - it looks like we readback using whatever context is associated with the image. Are you able to run this under a debugger and step through image->scalePixels to see if we hit any of the error cases? This might help us understand why the readback isn't working.
On 2016/11/17 21:42:20, cblume wrote: > On 2016/11/16 01:07:16, cblume wrote: > > This is currently erroring because two textures have different pixel values. > > > > The image obtained from getDeferredTextureImageData is returning white, which > is > > what we expect. > > > > The image that is failing is created from: > > > > SkAutoPixmapStorage scaled; > > scaled.alloc(scaled_info); > > image->scalePixels(scaled, testCase.fExpectedQuality); > > sk_sp<SkImage> scaledImage = SkImage::MakeRasterCopy(scaled); > > > > The variable 'image' is the one created from getDeferredTextureImageData in a > > separate context. > > > > Do we expect to be able to get a scaled image via readback this way? I need > some > > help on this part. > > Gentle ping, > I need help on this. I am unsure why this would be effected. Hey, sorry for being slow to respond! My guess is that we're getting a failure because we're trying to read back a texture on the wrong GL context. What if we remove the proxy from canGenerateTexture() and make the GPU image always return true? Would that make it so getDeferredTextureImageData would return false for this test case and the test would pass? I've found this all confusing, so apologies if I've given contradictory advice previously! My overall take is that we should not read back a cross-context texture in order to attempt to make getDTID succeed. In normal SkImage rendering we fail to draw when an SkImage from one GrContext is drawn to a SkSurface from another GrContext. So making getDTID fail seems consistent with that.
The CQ bit was checked by cblume@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-GN-Trybot on master.client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...) Build-Ubuntu-Clang-arm-Debug-GN_Android-Trybot on master.client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-Clang-a...) Build-Ubuntu-Clang-arm64-Debug-GN_Android-Trybot on master.client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-Clang-a...) Build-Ubuntu-Clang-arm64-Debug-GN_Android_FrameworkDefs-Trybot on master.client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-Clang-a...) Build-Ubuntu-Clang-x86_64-Debug-GN-Trybot on master.client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-Clang-x...) Build-Ubuntu-GCC-x86_64-Release-GN-Trybot on master.client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-x86...)
ping! :) What are the next steps on this CL?
On 2017/02/01 21:02:17, vmpstr wrote: > ping! :) What are the next steps on this CL? I put this on the backburner to focus on my OKRs. I had my priorities flipped. I've lost the mental context for this bug and will have to build it up again to answer. I'll do that once I wrap up my current task. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
