Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2014313002:
StreamTextureImages can now override a Texture's service id (Closed)
DescriptionStreamTextureImages can now override a Texture's service id
Previously Textures could have an unowned service id, which is used in
place of their service id, except that its lifetime is managed
externally. The use case this is AVDACodecImages, which use a single
SurfaceTexture to back multiple Textures.
This change more explicitly ties the unowned service id concept to
StreamTextureImages, and makes it possible to have StreamTextureImages
own the lifetime of the unowned service id (now called the stream
texture service id), by resetting the service id back to its original
value if a non-StreamTextureImage is bound. If this were not done and a
new image was bound to a level that previously had a
StreamTextureImage, the service id would still be the unowned stream
texture service id, but the StreamTextureImage would be deleted. That
meant it was previously not possible for StreamTextureImages to own the
lifetime of the unowned service id without possibly leaving the
Texture in an invalid state.
Now we can have multiple AVDACodecImages, bound to different Textures,
share a SurfaceTexture service id and safely delete the texture when all
of the AVDACodecImages are destructed.
This CL also adds Texture::SetLevelImageState for setting only the
ImageState and not changing the bound image.
BUG=614090
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
Committed: https://crrev.com/e8d35a561423eb3e4520b0ae1fe5a945a62c70af
Cr-Commit-Position: refs/heads/master@{#397897}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Partially implemented tying unowned id to StreamTextureImage #
Total comments: 2
Patch Set 3 : rebase only #Patch Set 4 : Only clear the stream texture service id if the image is changed #
Total comments: 13
Patch Set 5 : rebase #Patch Set 6 : liberato's comments #
Total comments: 2
Patch Set 7 : Added another test #
Total comments: 2
Patch Set 8 : Add SetLevelImageState #
Messages
Total messages: 29 (11 generated)
Description was changed from ========== Textures now clear their unowned service id when images are bound Textures can have an unowned service id which is used in place of their service id, except that its lifetime is managed externally. The use case for this is AVDACodecImages, which use a single SurfaceTexture to back multiple Textures. This change makes it easier to guarantee that Textures can't have invalid unowned ids by resetting them whenever an image is bound. Without this change you couldn't have a GLImage own the service id, because it would be possible for a new image to be bound, deleting the original image and leaving the Texture with an invalid unowned id. BUG=614090 ========== to ========== Textures now clear their unowned service id when images are bound Textures can have an unowned service id which is used in place of their service id, except that its lifetime is managed externally. The use case for this is AVDACodecImages, which use a single SurfaceTexture to back multiple Textures. This change makes it easier to guarantee that Textures can't have invalid unowned ids by resetting them whenever an image is bound. Without this change you couldn't have a GLImage own the service id, because it would be possible for a new image to be bound, deleting the original image and leaving the Texture with an invalid unowned id. BUG=614090 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel ==========
Description was changed from ========== Textures now clear their unowned service id when images are bound Textures can have an unowned service id which is used in place of their service id, except that its lifetime is managed externally. The use case for this is AVDACodecImages, which use a single SurfaceTexture to back multiple Textures. This change makes it easier to guarantee that Textures can't have invalid unowned ids by resetting them whenever an image is bound. Without this change you couldn't have a GLImage own the service id, because it would be possible for a new image to be bound, deleting the original image and leaving the Texture with an invalid unowned id. BUG=614090 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Textures now clear their unowned service id when images are bound Textures can have an unowned service id, which is used in place of their service id, except that its lifetime is managed externally. The use case for this is AVDACodecImages, which use a single SurfaceTexture to back multiple Textures. This change makes it easier to guarantee that Textures can't have invalid unowned ids by resetting them whenever an image is bound. Without this change you couldn't have a GLImage own the service id, because it would be possible for a new image to be bound, deleting the original image and leaving the Texture with an invalid unowned id. BUG=614090 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel ==========
watk@chromium.org changed reviewers: + piman@chromium.org
piman@ PTAL. This is a precursor to this CL: https://codereview.chromium.org/2005103004/ I wrote a bit of background here also, if you'd prefer to discuss there: https://docs.google.com/document/d/1u6MT8nQh-5Smq25BcuvYuHcmRY1jlgXh9cUZpawZB...
https://codereview.chromium.org/2014313002/diff/1/gpu/command_buffer/service/... File gpu/command_buffer/service/texture_manager.cc (right): https://codereview.chromium.org/2014313002/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/texture_manager.cc:1496: SetUnownedServiceId(0); I think I understand the semantics this is trying to achieve, but I wonder if we could make it a bit more explicit, by explicitly tying the unowned service id with the image, maybe by adding the unowned service id to SetLevelStreamTextureImage The idea is that this state becomes explicitly linked with the image - if you call SetLevelImage, it becomes implicitly 0, if you call SetLevelStreamTextureImage, it becomes whatever you pass it. As far as I can tell, the only (external) caller of SetUnownedServiceId is AndroidDeferredRenderingBackingStrategy::SetImageForPicture, which both set the service id and the image. Actually I think the current code would break without this, because that calls SetUnownedServiceId first, then SetLevelStreamTextureImage which would reset it. Thoughts?
On 2016/05/26 22:09:16, piman wrote: > https://codereview.chromium.org/2014313002/diff/1/gpu/command_buffer/service/... > File gpu/command_buffer/service/texture_manager.cc (right): > > https://codereview.chromium.org/2014313002/diff/1/gpu/command_buffer/service/... > gpu/command_buffer/service/texture_manager.cc:1496: SetUnownedServiceId(0); > I think I understand the semantics this is trying to achieve, but I wonder if we > could make it a bit more explicit, by explicitly tying the unowned service id > with the image, maybe by adding the unowned service id to > SetLevelStreamTextureImage > > The idea is that this state becomes explicitly linked with the image - if you > call SetLevelImage, it becomes implicitly 0, if you call > SetLevelStreamTextureImage, it becomes whatever you pass it. > > As far as I can tell, the only (external) caller of SetUnownedServiceId is > AndroidDeferredRenderingBackingStrategy::SetImageForPicture, which both set the > service id and the image. > > Actually I think the current code would break without this, because that calls > SetUnownedServiceId first, then SetLevelStreamTextureImage which would reset it. > Thoughts? I would love to make it more explicit! I'll upload a new CL to implement your suggestion next week. BTW, we're setting the image and unowned id in the opposite order of how you read it, so this should work as is.
On Thu, May 26, 2016 at 4:36 PM, <watk@chromium.org> wrote: > On 2016/05/26 22:09:16, piman wrote: > > > > https://codereview.chromium.org/2014313002/diff/1/gpu/command_buffer/service/... > > File gpu/command_buffer/service/texture_manager.cc (right): > > > > > > https://codereview.chromium.org/2014313002/diff/1/gpu/command_buffer/service/... > > gpu/command_buffer/service/texture_manager.cc:1496: > SetUnownedServiceId(0); > > I think I understand the semantics this is trying to achieve, but I > wonder if > we > > could make it a bit more explicit, by explicitly tying the unowned > service id > > with the image, maybe by adding the unowned service id to > > SetLevelStreamTextureImage > > > > The idea is that this state becomes explicitly linked with the image - > if you > > call SetLevelImage, it becomes implicitly 0, if you call > > SetLevelStreamTextureImage, it becomes whatever you pass it. > > > > As far as I can tell, the only (external) caller of SetUnownedServiceId > is > > AndroidDeferredRenderingBackingStrategy::SetImageForPicture, which both > set > the > > service id and the image. > > > > Actually I think the current code would break without this, because that > calls > > SetUnownedServiceId first, then SetLevelStreamTextureImage which would > reset > it. > > Thoughts? > > I would love to make it more explicit! I'll upload a new CL to implement > your > suggestion next week. > > BTW, we're setting the image and unowned id in the opposite order of how > you > read it, > so this should work as is. > Is that so? - We set the service id here: https://code.google.com/p/chromium/codesearch#chromium/src/media/gpu/android_... - We set the image here: https://code.google.com/p/chromium/codesearch#chromium/src/media/gpu/android_... What am I missing? > https://codereview.chromium.org/2014313002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/05/27 00:54:35, piman OOO back 2016-5-31 wrote: > On Thu, May 26, 2016 at 4:36 PM, <mailto:watk@chromium.org> wrote: > > > On 2016/05/26 22:09:16, piman wrote: > > > > > > > > https://codereview.chromium.org/2014313002/diff/1/gpu/command_buffer/service/... > > > File gpu/command_buffer/service/texture_manager.cc (right): > > > > > > > > > > > https://codereview.chromium.org/2014313002/diff/1/gpu/command_buffer/service/... > > > gpu/command_buffer/service/texture_manager.cc:1496: > > SetUnownedServiceId(0); > > > I think I understand the semantics this is trying to achieve, but I > > wonder if > > we > > > could make it a bit more explicit, by explicitly tying the unowned > > service id > > > with the image, maybe by adding the unowned service id to > > > SetLevelStreamTextureImage > > > > > > The idea is that this state becomes explicitly linked with the image - > > if you > > > call SetLevelImage, it becomes implicitly 0, if you call > > > SetLevelStreamTextureImage, it becomes whatever you pass it. > > > > > > As far as I can tell, the only (external) caller of SetUnownedServiceId > > is > > > AndroidDeferredRenderingBackingStrategy::SetImageForPicture, which both > > set > > the > > > service id and the image. > > > > > > Actually I think the current code would break without this, because that > > calls > > > SetUnownedServiceId first, then SetLevelStreamTextureImage which would > > reset > > it. > > > Thoughts? > > > > I would love to make it more explicit! I'll upload a new CL to implement > > your > > suggestion next week. > > > > BTW, we're setting the image and unowned id in the opposite order of how > > you > > read it, > > so this should work as is. > > > > Is that so? > - We set the service id here: > https://code.google.com/p/chromium/codesearch#chromium/src/media/gpu/android_... > > - We set the image here: > https://code.google.com/p/chromium/codesearch#chromium/src/media/gpu/android_... > > What am I missing? Wow, you're totally right, but I was sure I'd tested it. Either there's a bug or I didn't test it properly.
Uploaded some partial progress. I ran into a problem with the way we set the ImageState in GLES2DecoderImpl::DoCopyTexImage changing the bindings.
liberato@chromium.org changed reviewers: + liberato@chromium.org
https://codereview.chromium.org/2014313002/diff/20001/gpu/command_buffer/serv... File gpu/command_buffer/service/texture_manager.cc (right): https://codereview.chromium.org/2014313002/diff/20001/gpu/command_buffer/serv... gpu/command_buffer/service/texture_manager.cc:1503: SetStreamTextureServiceId(0); maybe just clear it if image != whatever is bound there now?
Patchset #3 (id:40001) has been deleted
https://codereview.chromium.org/2014313002/diff/20001/gpu/command_buffer/serv... File gpu/command_buffer/service/texture_manager.cc (right): https://codereview.chromium.org/2014313002/diff/20001/gpu/command_buffer/serv... gpu/command_buffer/service/texture_manager.cc:1503: SetStreamTextureServiceId(0); On 2016/06/01 14:10:44, liberato wrote: > maybe just clear it if image != whatever is bound there now? Sounds good. Done. The API is fairly consistent now I would say, except that we disregard levels when setting and resetting the stream texture service id, but it won't matter in practice. It's just a bit weird.
nice! -fl https://codereview.chromium.org/2014313002/diff/80001/gpu/command_buffer/serv... File gpu/command_buffer/service/texture_manager.cc (left): https://codereview.chromium.org/2014313002/diff/80001/gpu/command_buffer/serv... gpu/command_buffer/service/texture_manager.cc:1491: info.stream_texture_image = stream_texture_image; wow -- this was fragile before, and i just realized how bad it was. this kept setting and resetting the stream texture during normal drawing. DoCopyTexImage -> SetLevelImage -> here with |stream_texture_image| == nullptr, then avda_codec_image (hopefully) gets far enough in CopyTexImage to re-set it properly. that would leave the stream texture unset if CopyTexImage was called on an early-out case, such as CopyTextureCHROMIUM. i think that it would get back to normal on the next browser compositor frame, but the texture matrix wouldn't be set properly until it was. that happens before the associated CopyTexImage, so one frame would be wrong at least, i think. yay for if() :) https://codereview.chromium.org/2014313002/diff/80001/gpu/command_buffer/serv... File gpu/command_buffer/service/texture_manager.cc (right): https://codereview.chromium.org/2014313002/diff/80001/gpu/command_buffer/serv... gpu/command_buffer/service/texture_manager.cc:1492: if (info.image != image) { please update the tests (texture_manager_unittest.cc) for the new names, and include a to check for this case, and the new parameter in setLeveStreamTextureImage. there are already tests for the unowned service id to base it on. https://codereview.chromium.org/2014313002/diff/80001/gpu/command_buffer/serv... gpu/command_buffer/service/texture_manager.cc:1496: // different, so clear the stream texture service id because it's no longer nit: the comment is a little misleading. we don't know that it's no longer valid. it's because the stream texture manages the lifetime of the service id, and we can't guarantee that it's still valid. the extra context might be helpful just so folks realize that it's a security thing. https://codereview.chromium.org/2014313002/diff/80001/gpu/command_buffer/serv... File gpu/command_buffer/service/texture_manager.h (right): https://codereview.chromium.org/2014313002/diff/80001/gpu/command_buffer/serv... gpu/command_buffer/service/texture_manager.h:180: // reset to |owned_service_id_|. might want to mention "and is not equal to". https://codereview.chromium.org/2014313002/diff/80001/gpu/command_buffer/serv... gpu/command_buffer/service/texture_manager.h:195: GLuint service_id); i like this. nit: "will be unchanged" in the comment isn't correct -- it will be reverted to the owned service id if it wasn't before. probably just describe that it does a call to SetStreamTextureServiceId(service_id) as a convenience, then they'll go figure it out from the docs to that. no need to re-describe the actions of the latter here. https://codereview.chromium.org/2014313002/diff/80001/media/gpu/android_defer... File media/gpu/android_deferred_rendering_backing_strategy.cc (right): https://codereview.chromium.org/2014313002/diff/80001/media/gpu/android_defer... media/gpu/android_deferred_rendering_backing_strategy.cc:150: GLuint stream_texture_service_id = 0; just an aside, not part of this cl: we really shouldn't set the stream texture service id if !|surface_texture_|. i realize that we allocate it anyway in Initialize, but we shouldn't do that either :) https://codereview.chromium.org/2014313002/diff/80001/media/gpu/avda_codec_im... File media/gpu/avda_codec_image.cc (right): https://codereview.chromium.org/2014313002/diff/80001/media/gpu/avda_codec_im... media/gpu/avda_codec_image.cc:87: texture_->SetLevelStreamTextureImage( i think that this would be clearer as SetLevelImage, now that it checks for != old image.
Thanks for the comments! https://codereview.chromium.org/2014313002/diff/80001/gpu/command_buffer/serv... File gpu/command_buffer/service/texture_manager.cc (right): https://codereview.chromium.org/2014313002/diff/80001/gpu/command_buffer/serv... gpu/command_buffer/service/texture_manager.cc:1492: if (info.image != image) { On 2016/06/02 14:37:02, liberato wrote: > please update the tests (texture_manager_unittest.cc) for the new names, and > include a to check for this case, and the new parameter in > setLeveStreamTextureImage. there are already tests for the unowned service id > to base it on. Done. https://codereview.chromium.org/2014313002/diff/80001/gpu/command_buffer/serv... gpu/command_buffer/service/texture_manager.cc:1496: // different, so clear the stream texture service id because it's no longer On 2016/06/02 14:37:02, liberato wrote: > nit: the comment is a little misleading. we don't know that it's no longer > valid. it's because the stream texture manages the lifetime of the service id, > and we can't guarantee that it's still valid. the extra context might be > helpful just so folks realize that it's a security thing. Done. https://codereview.chromium.org/2014313002/diff/80001/gpu/command_buffer/serv... File gpu/command_buffer/service/texture_manager.h (right): https://codereview.chromium.org/2014313002/diff/80001/gpu/command_buffer/serv... gpu/command_buffer/service/texture_manager.h:180: // reset to |owned_service_id_|. On 2016/06/02 14:37:02, liberato wrote: > might want to mention "and is not equal to". Done. https://codereview.chromium.org/2014313002/diff/80001/gpu/command_buffer/serv... gpu/command_buffer/service/texture_manager.h:195: GLuint service_id); On 2016/06/02 14:37:02, liberato wrote: > i like this. > > nit: "will be unchanged" in the comment isn't correct -- it will be reverted to > the owned service id if it wasn't before. probably just describe that it does a > call to SetStreamTextureServiceId(service_id) as a convenience, then they'll go > figure it out from the docs to that. no need to re-describe the actions of the > latter here. Done. https://codereview.chromium.org/2014313002/diff/80001/media/gpu/android_defer... File media/gpu/android_deferred_rendering_backing_strategy.cc (right): https://codereview.chromium.org/2014313002/diff/80001/media/gpu/android_defer... media/gpu/android_deferred_rendering_backing_strategy.cc:150: GLuint stream_texture_service_id = 0; On 2016/06/02 14:37:02, liberato wrote: > just an aside, not part of this cl: we really shouldn't set the stream texture > service id if !|surface_texture_|. i realize that we allocate it anyway in > Initialize, but we shouldn't do that either :) Agreed. You'll be excited to see my next CL then :) https://codereview.chromium.org/2014313002/diff/80001/media/gpu/avda_codec_im... File media/gpu/avda_codec_image.cc (right): https://codereview.chromium.org/2014313002/diff/80001/media/gpu/avda_codec_im... media/gpu/avda_codec_image.cc:87: texture_->SetLevelStreamTextureImage( On 2016/06/02 14:37:02, liberato wrote: > i think that this would be clearer as SetLevelImage, now that it checks for != > old image. Done.
lgtm + suggestion for new test. thanks -fl https://codereview.chromium.org/2014313002/diff/120001/gpu/command_buffer/ser... File gpu/command_buffer/service/texture_manager_unittest.cc (right): https://codereview.chromium.org/2014313002/diff/120001/gpu/command_buffer/ser... gpu/command_buffer/service/texture_manager_unittest.cc:1737: TEST_F(TextureTest, OverrideServiceID) { can you add a new test that checks that SetLevelImage does not reset service_id() to |owned| when the image is the same, but does reset it when it's a different image?
https://codereview.chromium.org/2014313002/diff/140001/gpu/command_buffer/ser... File gpu/command_buffer/service/texture_manager.cc (right): https://codereview.chromium.org/2014313002/diff/140001/gpu/command_buffer/ser... gpu/command_buffer/service/texture_manager.cc:1513: if (info.image != image) { I find this semantic a bit odd and hard to reason about (I don't know whether SetLevelImage will reset the service_id unless I already know abou tthe current image)... Is it that there is some code that only wants to change the ImageState? Would it make sense instead to add a SetLevelImageState method that only changes the image_state, and have SetLevelImage unconditionally reset the service_id?
Description was changed from ========== Textures now clear their unowned service id when images are bound Textures can have an unowned service id, which is used in place of their service id, except that its lifetime is managed externally. The use case for this is AVDACodecImages, which use a single SurfaceTexture to back multiple Textures. This change makes it easier to guarantee that Textures can't have invalid unowned ids by resetting them whenever an image is bound. Without this change you couldn't have a GLImage own the service id, because it would be possible for a new image to be bound, deleting the original image and leaving the Texture with an invalid unowned id. BUG=614090 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== StreamTextureImages can now override a Texture's service id Previously Textures could have an unowned service id, which is used in place of their service id, except that its lifetime is managed externally. The use case this is AVDACodecImages, which use a single SurfaceTexture to back multiple Textures. This change more explicitly ties the unowned service id concept to StreamTextureImages, and makes it possible to have StreamTextureImages own the lifetime of the unowned service id (now called the stream texture service id), by resetting the service id back to its original value if a StreamTextureImage is unbound. If this were not done and a new image was bound to a level that previously had a StreamTextureImage, the service id would still be the unowned stream texture service id, but the StreamTextureImage would be deleted. So, it was previously not possible for StreamTextureImages to own the lifetime of the unowned service id without possibly leaving the Texture in an invalid state. Now we can have multiple Textures, with AVDACodecImages bound, share a SurfaceTexture service id and safely delete the texture when all of the AVDACodecImages are destructed. BUG=614090 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel ==========
https://codereview.chromium.org/2014313002/diff/120001/gpu/command_buffer/ser... File gpu/command_buffer/service/texture_manager_unittest.cc (right): https://codereview.chromium.org/2014313002/diff/120001/gpu/command_buffer/ser... gpu/command_buffer/service/texture_manager_unittest.cc:1737: TEST_F(TextureTest, OverrideServiceID) { On 2016/06/03 14:51:56, liberato wrote: > can you add a new test that checks that SetLevelImage does not reset > service_id() to |owned| when the image is the same, but does reset it when it's > a different image? Done. https://codereview.chromium.org/2014313002/diff/140001/gpu/command_buffer/ser... File gpu/command_buffer/service/texture_manager.cc (right): https://codereview.chromium.org/2014313002/diff/140001/gpu/command_buffer/ser... gpu/command_buffer/service/texture_manager.cc:1513: if (info.image != image) { On 2016/06/03 20:32:35, piman OOO back 2016-6-2 wrote: > I find this semantic a bit odd and hard to reason about (I don't know whether > SetLevelImage will reset the service_id unless I already know abou tthe current > image)... Is it that there is some code that only wants to change the > ImageState? Would it make sense instead to add a SetLevelImageState method that > only changes the image_state, and have SetLevelImage unconditionally reset the > service_id? Yes, and yes I think so. Will do.
Description was changed from ========== StreamTextureImages can now override a Texture's service id Previously Textures could have an unowned service id, which is used in place of their service id, except that its lifetime is managed externally. The use case this is AVDACodecImages, which use a single SurfaceTexture to back multiple Textures. This change more explicitly ties the unowned service id concept to StreamTextureImages, and makes it possible to have StreamTextureImages own the lifetime of the unowned service id (now called the stream texture service id), by resetting the service id back to its original value if a StreamTextureImage is unbound. If this were not done and a new image was bound to a level that previously had a StreamTextureImage, the service id would still be the unowned stream texture service id, but the StreamTextureImage would be deleted. So, it was previously not possible for StreamTextureImages to own the lifetime of the unowned service id without possibly leaving the Texture in an invalid state. Now we can have multiple Textures, with AVDACodecImages bound, share a SurfaceTexture service id and safely delete the texture when all of the AVDACodecImages are destructed. BUG=614090 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== StreamTextureImages can now override a Texture's service id Previously Textures could have an unowned service id, which is used in place of their service id, except that its lifetime is managed externally. The use case this is AVDACodecImages, which use a single SurfaceTexture to back multiple Textures. This change more explicitly ties the unowned service id concept to StreamTextureImages, and makes it possible to have StreamTextureImages own the lifetime of the unowned service id (now called the stream texture service id), by resetting the service id back to its original value if a non-StreamTextureImage is bound. If this were not done and a new image was bound to a level that previously had a StreamTextureImage, the service id would still be the unowned stream texture service id, but the StreamTextureImage would be deleted. That meant it was previously not possible for StreamTextureImages to own the lifetime of the unowned service id without possibly leaving the Texture in an invalid state. Now we can have multiple AVDACodecImages, bound to different Textures, share a SurfaceTexture service id and safely delete the texture when all of the AVDACodecImages are destructed. This CL also adds Texture::SetLevelImageState for setting only the ImageState and not changing the bound image. BUG=614090 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel ==========
PTAL. This works in my simple testing. Any particular cases I should try Frank?
LGTM, thanks!
The CQ bit was checked by watk@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from liberato@chromium.org Link to the patchset: https://codereview.chromium.org/2014313002/#ps160001 (title: "Add SetLevelImageState")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2014313002/160001
Message was sent while issue was closed.
Description was changed from ========== StreamTextureImages can now override a Texture's service id Previously Textures could have an unowned service id, which is used in place of their service id, except that its lifetime is managed externally. The use case this is AVDACodecImages, which use a single SurfaceTexture to back multiple Textures. This change more explicitly ties the unowned service id concept to StreamTextureImages, and makes it possible to have StreamTextureImages own the lifetime of the unowned service id (now called the stream texture service id), by resetting the service id back to its original value if a non-StreamTextureImage is bound. If this were not done and a new image was bound to a level that previously had a StreamTextureImage, the service id would still be the unowned stream texture service id, but the StreamTextureImage would be deleted. That meant it was previously not possible for StreamTextureImages to own the lifetime of the unowned service id without possibly leaving the Texture in an invalid state. Now we can have multiple AVDACodecImages, bound to different Textures, share a SurfaceTexture service id and safely delete the texture when all of the AVDACodecImages are destructed. This CL also adds Texture::SetLevelImageState for setting only the ImageState and not changing the bound image. BUG=614090 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== StreamTextureImages can now override a Texture's service id Previously Textures could have an unowned service id, which is used in place of their service id, except that its lifetime is managed externally. The use case this is AVDACodecImages, which use a single SurfaceTexture to back multiple Textures. This change more explicitly ties the unowned service id concept to StreamTextureImages, and makes it possible to have StreamTextureImages own the lifetime of the unowned service id (now called the stream texture service id), by resetting the service id back to its original value if a non-StreamTextureImage is bound. If this were not done and a new image was bound to a level that previously had a StreamTextureImage, the service id would still be the unowned stream texture service id, but the StreamTextureImage would be deleted. That meant it was previously not possible for StreamTextureImages to own the lifetime of the unowned service id without possibly leaving the Texture in an invalid state. Now we can have multiple AVDACodecImages, bound to different Textures, share a SurfaceTexture service id and safely delete the texture when all of the AVDACodecImages are destructed. This CL also adds Texture::SetLevelImageState for setting only the ImageState and not changing the bound image. BUG=614090 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel ==========
Message was sent while issue was closed.
Committed patchset #8 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== StreamTextureImages can now override a Texture's service id Previously Textures could have an unowned service id, which is used in place of their service id, except that its lifetime is managed externally. The use case this is AVDACodecImages, which use a single SurfaceTexture to back multiple Textures. This change more explicitly ties the unowned service id concept to StreamTextureImages, and makes it possible to have StreamTextureImages own the lifetime of the unowned service id (now called the stream texture service id), by resetting the service id back to its original value if a non-StreamTextureImage is bound. If this were not done and a new image was bound to a level that previously had a StreamTextureImage, the service id would still be the unowned stream texture service id, but the StreamTextureImage would be deleted. That meant it was previously not possible for StreamTextureImages to own the lifetime of the unowned service id without possibly leaving the Texture in an invalid state. Now we can have multiple AVDACodecImages, bound to different Textures, share a SurfaceTexture service id and safely delete the texture when all of the AVDACodecImages are destructed. This CL also adds Texture::SetLevelImageState for setting only the ImageState and not changing the bound image. BUG=614090 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== StreamTextureImages can now override a Texture's service id Previously Textures could have an unowned service id, which is used in place of their service id, except that its lifetime is managed externally. The use case this is AVDACodecImages, which use a single SurfaceTexture to back multiple Textures. This change more explicitly ties the unowned service id concept to StreamTextureImages, and makes it possible to have StreamTextureImages own the lifetime of the unowned service id (now called the stream texture service id), by resetting the service id back to its original value if a non-StreamTextureImage is bound. If this were not done and a new image was bound to a level that previously had a StreamTextureImage, the service id would still be the unowned stream texture service id, but the StreamTextureImage would be deleted. That meant it was previously not possible for StreamTextureImages to own the lifetime of the unowned service id without possibly leaving the Texture in an invalid state. Now we can have multiple AVDACodecImages, bound to different Textures, share a SurfaceTexture service id and safely delete the texture when all of the AVDACodecImages are destructed. This CL also adds Texture::SetLevelImageState for setting only the ImageState and not changing the bound image. BUG=614090 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel Committed: https://crrev.com/e8d35a561423eb3e4520b0ae1fe5a945a62c70af Cr-Commit-Position: refs/heads/master@{#397897} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/e8d35a561423eb3e4520b0ae1fe5a945a62c70af Cr-Commit-Position: refs/heads/master@{#397897} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
