Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1924183003:
Make stencils be attachable to render targets created via SkSurface::MakeFromBackendTextureAsRender… (Closed)
|
Created:
4 years, 7 months ago by Kimmo Kinnunen Modified:
4 years, 7 months ago Reviewers:
bsalomon CC:
reviews_skia.org Base URL:
https://skia.googlesource.com/skia.git@master Target Ref:
refs/heads/master Project:
skia Visibility:
Public. |
DescriptionMake stencils be attachable to render targets created via SkSurface::MakeFromBackendTextureAsRenderTarget
This is a regression from "Refactor to separate backend object lifecycle
and GpuResource budget decision".
GrGLRenderTarget::CreateWrapped creates only render targets that wrap
the FBO.
GrGLRenderTargetTexture::CreateWrapped creates render targets that
wrap the texture.
Use the latter as the implementation for
SkSurface::MakeFromBackendTextureAsRenderTarget.
BUG=608238
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1924183003
Committed: https://skia.googlesource.com/skia/+/fe635fd76bbc375d527d1884af23cc617ca364ee
Committed: https://skia.googlesource.com/skia/+/1cb00e42bff455e23f6883bf09680438fa631224
Patch Set 1 #Patch Set 2 : #Patch Set 3 : use if (sampleCnt == 0) #Patch Set 4 : #Patch Set 5 : less risky #Patch Set 6 : retry the test #Patch Set 7 : debug for debug #Patch Set 8 : remove the shape check from the test #Patch Set 9 : #
Messages
Total messages: 32 (13 generated)
Description was changed from ========== Make stencils be attachable to render targets created via SkSurface::MakeFromBackendTextureAsRenderTarget This is a regression from "Refactor to separate backend object lifecycle and GpuResource budget decision". GrGLRenderTarget::CreateWrapped creates only render targets that wrap the FBO. GrGLRenderTargetTexture::CreateWrapped creates render targets that wrap the texture. Use the latter as the implementation for SkSurface::MakeFromBackendTextureAsRenderTarget. The test contains disabled code. The MakeFromBackendTextureAsRenderTarget does not copy the existing texture contents to the FBO render buffer. ========== to ========== Make stencils be attachable to render targets created via SkSurface::MakeFromBackendTextureAsRenderTarget This is a regression from "Refactor to separate backend object lifecycle and GpuResource budget decision". GrGLRenderTarget::CreateWrapped creates only render targets that wrap the FBO. GrGLRenderTargetTexture::CreateWrapped creates render targets that wrap the texture. Use the latter as the implementation for SkSurface::MakeFromBackendTextureAsRenderTarget. The test contains disabled code. The MakeFromBackendTextureAsRenderTarget does not copy the existing texture contents to the FBO render buffer. GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
kkinnunen@nvidia.com changed reviewers: + bsalomon@google.com
The CQ bit was checked by kkinnunen@nvidia.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1924183003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1924183003/1
Does the commented out test fail only when sample count is non-zero? I'm a bit hesitant to add a blit that many clients would probably not want. We could take a clear/preserve param, or always clear and document that behavior.
On 2016/04/28 14:12:26, bsalomon wrote: > Does the commented out test fail only when sample count is non-zero? > > I'm a bit hesitant to add a blit that many clients would probably not want. We > could take a clear/preserve param, or always clear and document that behavior. Or just document that if sample count > 0 the existing contents aren't preserved.
On 2016/04/28 14:12:26, bsalomon wrote: > Does the commented out test fail only when sample count is non-zero? > > I'm a bit hesitant to add a blit that many clients would probably not want. We > could take a clear/preserve param, or always clear and document that behavior. Sample count non-zero and platform non-multisampled-render-to-texture (this patch does not try to address this bug)
On 2016/04/28 14:37:37, Kimmo Kinnunen wrote: > On 2016/04/28 14:12:26, bsalomon wrote: > > Does the commented out test fail only when sample count is non-zero? > > > > I'm a bit hesitant to add a blit that many clients would probably not want. We > > could take a clear/preserve param, or always clear and document that behavior. > > Sample count non-zero and platform non-multisampled-render-to-texture > > (this patch does not try to address this bug) Ok, I propose enabling the test for sample count zero and updating the documentation on SkSurface::MakeFromBackendTexture and ::MakeFromBackendTextureAsRenderTarget that it is undefined whether previous contents are preserved if sample count is greater than zero. Until we have a client that cares about preserving the contents I'm happy to define the bug away.
On 2016/04/28 15:00:49, bsalomon wrote: > Ok, I propose enabling the test for sample count zero and updating the > documentation on SkSurface::MakeFromBackendTexture and > ::MakeFromBackendTextureAsRenderTarget that it is undefined whether previous > contents are preserved if sample count is greater than zero. Until we have a > client that cares about preserving the contents I'm happy to define the bug > away. Sure. May I do that in a different patch? It's not really part of this change at all. I can remove the code that is commented out or something?
On 2016/04/28 15:19:38, Kimmo Kinnunen wrote: > On 2016/04/28 15:00:49, bsalomon wrote: > > Ok, I propose enabling the test for sample count zero and updating the > > documentation on SkSurface::MakeFromBackendTexture and > > ::MakeFromBackendTextureAsRenderTarget that it is undefined whether previous > > contents are preserved if sample count is greater than zero. Until we have a > > client that cares about preserving the contents I'm happy to define the bug > > away. > > Sure. May I do that in a different patch? It's not really part of this change at > all. > I can remove the code that is commented out or something? Sure, lgtm. I'd either remove it or just put it in a if (samplecount > 0) check.
The CQ bit was checked by kkinnunen@nvidia.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1924183003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1924183003/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Build-Ubuntu-GCC-Arm64-Debug-Android-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Arm...) Build-Ubuntu-GCC-Mips-Debug-Android-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Mip...) Build-Ubuntu-GCC-x86_64-Release-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-x86...)
The CQ bit was checked by kkinnunen@nvidia.com
The patchset sent to the CQ was uploaded after l-g-t-m from bsalomon@google.com Link to the patchset: https://codereview.chromium.org/1924183003/#ps80001 (title: "less risky")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1924183003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1924183003/80001
Message was sent while issue was closed.
Description was changed from ========== Make stencils be attachable to render targets created via SkSurface::MakeFromBackendTextureAsRenderTarget This is a regression from "Refactor to separate backend object lifecycle and GpuResource budget decision". GrGLRenderTarget::CreateWrapped creates only render targets that wrap the FBO. GrGLRenderTargetTexture::CreateWrapped creates render targets that wrap the texture. Use the latter as the implementation for SkSurface::MakeFromBackendTextureAsRenderTarget. The test contains disabled code. The MakeFromBackendTextureAsRenderTarget does not copy the existing texture contents to the FBO render buffer. GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Make stencils be attachable to render targets created via SkSurface::MakeFromBackendTextureAsRenderTarget This is a regression from "Refactor to separate backend object lifecycle and GpuResource budget decision". GrGLRenderTarget::CreateWrapped creates only render targets that wrap the FBO. GrGLRenderTargetTexture::CreateWrapped creates render targets that wrap the texture. Use the latter as the implementation for SkSurface::MakeFromBackendTextureAsRenderTarget. The test contains disabled code. The MakeFromBackendTextureAsRenderTarget does not copy the existing texture contents to the FBO render buffer. GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/fe635fd76bbc375d527d1884af23cc617ca364ee ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://skia.googlesource.com/skia/+/fe635fd76bbc375d527d1884af23cc617ca364ee
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:80001) has been created in https://codereview.chromium.org/1931293003/ by benjaminwagner@google.com. The reason for reverting is: Seems to be causing failure in DM on some Windows bots: c:\0\build\slave\workdir\build\skia\tests\surfacetest.cpp:963 kShapeColor == bitmap.getColor(kW / 2, kH / 2) .
Message was sent while issue was closed.
Description was changed from ========== Make stencils be attachable to render targets created via SkSurface::MakeFromBackendTextureAsRenderTarget This is a regression from "Refactor to separate backend object lifecycle and GpuResource budget decision". GrGLRenderTarget::CreateWrapped creates only render targets that wrap the FBO. GrGLRenderTargetTexture::CreateWrapped creates render targets that wrap the texture. Use the latter as the implementation for SkSurface::MakeFromBackendTextureAsRenderTarget. The test contains disabled code. The MakeFromBackendTextureAsRenderTarget does not copy the existing texture contents to the FBO render buffer. GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/fe635fd76bbc375d527d1884af23cc617ca364ee ========== to ========== Make stencils be attachable to render targets created via SkSurface::MakeFromBackendTextureAsRenderTarget This is a regression from "Refactor to separate backend object lifecycle and GpuResource budget decision". GrGLRenderTarget::CreateWrapped creates only render targets that wrap the FBO. GrGLRenderTargetTexture::CreateWrapped creates render targets that wrap the texture. Use the latter as the implementation for SkSurface::MakeFromBackendTextureAsRenderTarget. The test contains disabled code. The MakeFromBackendTextureAsRenderTarget does not copy the existing texture contents to the FBO render buffer. GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/fe635fd76bbc375d527d1884af23cc617ca364ee ==========
Description was changed from ========== Make stencils be attachable to render targets created via SkSurface::MakeFromBackendTextureAsRenderTarget This is a regression from "Refactor to separate backend object lifecycle and GpuResource budget decision". GrGLRenderTarget::CreateWrapped creates only render targets that wrap the FBO. GrGLRenderTargetTexture::CreateWrapped creates render targets that wrap the texture. Use the latter as the implementation for SkSurface::MakeFromBackendTextureAsRenderTarget. The test contains disabled code. The MakeFromBackendTextureAsRenderTarget does not copy the existing texture contents to the FBO render buffer. GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/fe635fd76bbc375d527d1884af23cc617ca364ee ========== to ========== Make stencils be attachable to render targets created via SkSurface::MakeFromBackendTextureAsRenderTarget This is a regression from "Refactor to separate backend object lifecycle and GpuResource budget decision". GrGLRenderTarget::CreateWrapped creates only render targets that wrap the FBO. GrGLRenderTargetTexture::CreateWrapped creates render targets that wrap the texture. Use the latter as the implementation for SkSurface::MakeFromBackendTextureAsRenderTarget. BUG=608238 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/fe635fd76bbc375d527d1884af23cc617ca364ee ==========
Brian, would it be possible to commit this without the shape check in the test? I have hard time obtaining the HW needed for this, and I can not debug this with printfs, they don't seem to end up in the buildbot output on windows.. I changed the tests, took them from the doc commit and tried to preserve the types..
On 2016/05/02 17:44:43, Kimmo Kinnunen wrote: > Brian, would it be possible to commit this without the shape check in the test? > I have hard time obtaining the HW needed for this, and I can not debug this with > printfs, they don't seem to end up in the buildbot output on windows.. > > I changed the tests, took them from the doc commit and tried to preserve the > types.. I can try to add the shape check back later, if I ever can repro the problem...
On 2016/05/02 17:45:40, Kimmo Kinnunen wrote: > On 2016/05/02 17:44:43, Kimmo Kinnunen wrote: > > Brian, would it be possible to commit this without the shape check in the > test? > > I have hard time obtaining the HW needed for this, and I can not debug this > with > > printfs, they don't seem to end up in the buildbot output on windows.. > > > > I changed the tests, took them from the doc commit and tried to preserve the > > types.. > > I can try to add the shape check back later, if I ever can repro the problem... I'm ok with that.
The CQ bit was checked by kkinnunen@nvidia.com
The patchset sent to the CQ was uploaded after l-g-t-m from bsalomon@google.com Link to the patchset: https://codereview.chromium.org/1924183003/#ps150001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1924183003/150001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1924183003/150001
Message was sent while issue was closed.
Description was changed from ========== Make stencils be attachable to render targets created via SkSurface::MakeFromBackendTextureAsRenderTarget This is a regression from "Refactor to separate backend object lifecycle and GpuResource budget decision". GrGLRenderTarget::CreateWrapped creates only render targets that wrap the FBO. GrGLRenderTargetTexture::CreateWrapped creates render targets that wrap the texture. Use the latter as the implementation for SkSurface::MakeFromBackendTextureAsRenderTarget. BUG=608238 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/fe635fd76bbc375d527d1884af23cc617ca364ee ========== to ========== Make stencils be attachable to render targets created via SkSurface::MakeFromBackendTextureAsRenderTarget This is a regression from "Refactor to separate backend object lifecycle and GpuResource budget decision". GrGLRenderTarget::CreateWrapped creates only render targets that wrap the FBO. GrGLRenderTargetTexture::CreateWrapped creates render targets that wrap the texture. Use the latter as the implementation for SkSurface::MakeFromBackendTextureAsRenderTarget. BUG=608238 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/fe635fd76bbc375d527d1884af23cc617ca364ee Committed: https://skia.googlesource.com/skia/+/1cb00e42bff455e23f6883bf09680438fa631224 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:150001) as https://skia.googlesource.com/skia/+/1cb00e42bff455e23f6883bf09680438fa631224
Message was sent while issue was closed.
A revert of this CL (patchset #9 id:150001) has been created in https://codereview.chromium.org/1943843002/ by jvanverth@google.com. The reason for reverting is: Appears to be breaking the roll.. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
