Return scoped_refptr<VASurface> not VASurfaceID in VaapiWrapper::CreateSurfaces |
|||||
Issue descriptionThere are two ways to create VASurfaceID with VaapiWrapper. 1.) CreateVASurfaceForPixmap: this returns scoped_refptr<VASurface> and VASurfaceID of the surface is not stored in va_surface_ids_ in VaapiWrapper. 2.) CreateSurfaces(): this returns VASurfaceIDs and they are also stored in va_surface_ids_ in VaapiWrapper. In other words, their owner is VaapiWrapper. It is a little bit confusing that the VASurface's ownership is different by the way of creating. Those VASurfaceIDs are always processed in the same way in other places. I think CreateSurface() should return scoped_refptr<VASurface> and the caller owns them. We can remove va_surface_ids_ and DestroySurfaces() in this way.
,
Nov 28
It sounds good to me to homogenize the surface circulation. This change might need to go a bit deep because [1] it'll imply a slight change in ownership, but I'm all in for clients doing that and leaving VaapiWrapper to "just" serialize access to libva. [1] https://cs.chromium.org/chromium/src/media/gpu/vaapi/vaapi_wrapper.h?type=cs&q=CreateSurfaces+file:%5Esrc/media/gpu/vaapi/+package:%5Echromium$&g=0&l=120
,
Nov 28
,
Nov 29
Good idea! This was on my list of things to do, thanks for creating a bug!
,
Nov 29
Thanks david. Let me assign you. Thanks.
,
Nov 29
Thanks, I'll try to do this sometime next week!
,
Nov 29
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/04fbdd9f175a350b7a674ab0610c3963428e0e1d commit 04fbdd9f175a350b7a674ab0610c3963428e0e1d Author: Hirokazu Honda <hiroh@chromium.org> Date: Thu Nov 29 08:31:34 2018 media/gpu/vaapi/vaapiWrapper: Acquire lock in CreateContext() and DestroySurfaces() CreateContext() doesn't acquire lock if it is called by VaapiWrapper's caller. Lock should be acquired always in CreateContext(). This change also removes DestroySurfaces_Locked() and instead DestroySurfaces() is called everywhere. CreateSurfaces() and DestroySurfaces() are renamed to CreateContextAndSurfaces() and DestroyContextAndSurfaces() to specify what they are doing. Bug: 909547 , 909561 Test: VDA, VEA, JDA, JEA unittest on eve Change-Id: Ib04386307c2e83d83e0afe079a1868ac02eff086 Reviewed-on: https://chromium-review.googlesource.com/c/1353056 Commit-Queue: Hirokazu Honda <hiroh@chromium.org> Reviewed-by: Miguel Casas <mcasas@chromium.org> Cr-Commit-Position: refs/heads/master@{#612101} [modify] https://crrev.com/04fbdd9f175a350b7a674ab0610c3963428e0e1d/media/gpu/vaapi/vaapi_jpeg_decode_accelerator.cc [modify] https://crrev.com/04fbdd9f175a350b7a674ab0610c3963428e0e1d/media/gpu/vaapi/vaapi_jpeg_decode_accelerator_unittest.cc [modify] https://crrev.com/04fbdd9f175a350b7a674ab0610c3963428e0e1d/media/gpu/vaapi/vaapi_jpeg_encode_accelerator.cc [modify] https://crrev.com/04fbdd9f175a350b7a674ab0610c3963428e0e1d/media/gpu/vaapi/vaapi_video_decode_accelerator.cc [modify] https://crrev.com/04fbdd9f175a350b7a674ab0610c3963428e0e1d/media/gpu/vaapi/vaapi_video_decode_accelerator_unittest.cc [modify] https://crrev.com/04fbdd9f175a350b7a674ab0610c3963428e0e1d/media/gpu/vaapi/vaapi_video_encode_accelerator.cc [modify] https://crrev.com/04fbdd9f175a350b7a674ab0610c3963428e0e1d/media/gpu/vaapi/vaapi_wrapper.cc [modify] https://crrev.com/04fbdd9f175a350b7a674ab0610c3963428e0e1d/media/gpu/vaapi/vaapi_wrapper.h
,
Nov 30
,
Dec 3
,
Dec 14
I investigated adapting CreateContextAndSurfaces to return VASurface objects rather than raw id's. Unfortunately it's not an easy change: - Multiple surfaces are created together with a context, and should be destroyed together. - The VASurface object is also used by the VAAPI video decoder. In this case the destroy method is overwritten to return the surface to a list of available surfaces. This inconsistency should be fixed first. As we're planning on migrating to the media::VideoDecoder interface. It might make sense to hold off on this change for now, until we simplified the code by migrating to the new interface. |
|||||
►
Sign in to add a comment |
|||||
Comment 1 by hiroh@chromium.org
, Nov 28