Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1432963003:
[Ozone] Extends the lifetime of VaapiWrapper (Closed)
|
Created:
5 years, 1 month ago by william.xie1 Modified:
5 years ago CC:
chromium-reviews, ozone-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Ozone] Extends the lifetime of VaapiWrapper
Currently when reloading webpage during video playback on overlay,
VaapiWrapper is destroyed immediately, thus the callback of processing
pixmap is getting invalid. This CL extends the lifetime of VaapiWrapper
to ensure pixmaps being processed.
BUG=554005
Committed: https://crrev.com/484a828e7021821546a6728eeba02792941a20ce
Cr-Commit-Position: refs/heads/master@{#362650}
Patch Set 1 #Patch Set 2 : #
Total comments: 4
Patch Set 3 : NOT for upstream #
Total comments: 20
Patch Set 4 : #Patch Set 5 : Rebase #Patch Set 6 : break the circulating reference #
Total comments: 1
Patch Set 7 : #
Total comments: 3
Patch Set 8 : Extends the lifetime of VaapiWrapper #
Total comments: 24
Patch Set 9 : #
Total comments: 5
Patch Set 10 : #Patch Set 11 : #Patch Set 12 : #Patch Set 13 : fix build issue #
Total comments: 4
Patch Set 14 : nits/API naming fix #
Total comments: 2
Patch Set 15 : #Patch Set 16 : rebase #
Total comments: 9
Patch Set 17 : #Patch Set 18 : Rebase #
Total comments: 2
Patch Set 19 : #Messages
Total messages: 122 (39 generated)
william.xie@intel.com changed reviewers: + alexst@chromium.org, piman@chromium.org, reveman@chromium.org, spang@chromium.org
PTAL
On 2015/11/10 14:37:27, william.xie wrote: > PTAL I don't think it is the right fix. The code shouldn't call ScheduleOverlayPlane() after Destroy(). We should fix the code that is doing that.
PTAL
https://codereview.chromium.org/1432963003/diff/20001/ui/gl/gl_surface_ozone.cc File ui/gl/gl_surface_ozone.cc (right): https://codereview.chromium.org/1432963003/diff/20001/ui/gl/gl_surface_ozone.... ui/gl/gl_surface_ozone.cc:227: bool GLSurfaceOzoneSurfaceless::Overlay::ScheduleOverlayPlane( why is this called after the image has been destroyed?
https://codereview.chromium.org/1432963003/diff/20001/ui/gl/gl_surface_ozone.cc File ui/gl/gl_surface_ozone.cc (right): https://codereview.chromium.org/1432963003/diff/20001/ui/gl/gl_surface_ozone.... ui/gl/gl_surface_ozone.cc:227: bool GLSurfaceOzoneSurfaceless::Overlay::ScheduleOverlayPlane( On 2015/11/12 14:58:26, reveman wrote: > why is this called after the image has been destroyed? the image is in the unsubmitted_frames_, and will be scheduled to overlay when swapbuffer. During video playback on overlay, if user reload the webpage, the mediaplayer pipeline will be destroyed immediately, and the image will be destroyed too. However, the image object itself is still valid as it is cached in pending frames, so this CL fixes the issue by checking if the image is still valid before scheduling it to overlay. Just let me know if the explanation is not clear.
https://codereview.chromium.org/1432963003/diff/20001/ui/gl/gl_surface_ozone.cc File ui/gl/gl_surface_ozone.cc (right): https://codereview.chromium.org/1432963003/diff/20001/ui/gl/gl_surface_ozone.... ui/gl/gl_surface_ozone.cc:227: bool GLSurfaceOzoneSurfaceless::Overlay::ScheduleOverlayPlane( On 2015/11/12 15:37:43, william.xie wrote: > On 2015/11/12 14:58:26, reveman wrote: > > why is this called after the image has been destroyed? > > the image is in the unsubmitted_frames_, and will be scheduled to overlay when > swapbuffer. During video playback on overlay, if user reload the webpage, the > mediaplayer pipeline will be destroyed immediately, and the image will be > destroyed too. However, the image object itself is still valid as it is cached > in pending frames, so this CL fixes the issue by checking if the image is still > valid before scheduling it to overlay. > > Just let me know if the explanation is not clear. If it was totally valid when scheduled, we can't just drop it on the floor here. Probably unsubmitted_frames_ queuing logic should be moved to GbmSurfaceless. Need to find a way to do the fence bits though.
On 2015/11/12 15:47:53, spang wrote: > https://codereview.chromium.org/1432963003/diff/20001/ui/gl/gl_surface_ozone.cc > File ui/gl/gl_surface_ozone.cc (right): > > https://codereview.chromium.org/1432963003/diff/20001/ui/gl/gl_surface_ozone.... > ui/gl/gl_surface_ozone.cc:227: bool > GLSurfaceOzoneSurfaceless::Overlay::ScheduleOverlayPlane( > On 2015/11/12 15:37:43, william.xie wrote: > > On 2015/11/12 14:58:26, reveman wrote: > > > why is this called after the image has been destroyed? > > > > the image is in the unsubmitted_frames_, and will be scheduled to overlay when > > swapbuffer. During video playback on overlay, if user reload the webpage, the > > mediaplayer pipeline will be destroyed immediately, and the image will be > > destroyed too. However, the image object itself is still valid as it is cached > > in pending frames, so this CL fixes the issue by checking if the image is > still > > valid before scheduling it to overlay. > > > > Just let me know if the explanation is not clear. > > If it was totally valid when scheduled, we can't just drop it on the floor here. > > Probably unsubmitted_frames_ queuing logic should be moved to GbmSurfaceless. > Need to find a way to do the fence bits though. That's good idea, Michael, do you mean GLFence? ChromiumOS supports b_EGL_KHR_fence_sync, right?
kalyan.kondapally@intel.com changed reviewers: + kalyan.kondapally@intel.com
https://codereview.chromium.org/1432963003/diff/20001/ui/gl/gl_surface_ozone.cc File ui/gl/gl_surface_ozone.cc (right): https://codereview.chromium.org/1432963003/diff/20001/ui/gl/gl_surface_ozone.... ui/gl/gl_surface_ozone.cc:227: bool GLSurfaceOzoneSurfaceless::Overlay::ScheduleOverlayPlane( On 2015/11/12 15:47:52, spang wrote: > On 2015/11/12 15:37:43, william.xie wrote: > > On 2015/11/12 14:58:26, reveman wrote: > > > why is this called after the image has been destroyed? > > > > the image is in the unsubmitted_frames_, and will be scheduled to overlay when > > swapbuffer. During video playback on overlay, if user reload the webpage, the > > mediaplayer pipeline will be destroyed immediately, and the image will be > > destroyed too. However, the image object itself is still valid as it is cached > > in pending frames, so this CL fixes the issue by checking if the image is > still > > valid before scheduling it to overlay. > > > > Just let me know if the explanation is not clear. > > If it was totally valid when scheduled, we can't just drop it on the floor here. > > Probably unsubmitted_frames_ queuing logic should be moved to GbmSurfaceless. > Need to find a way to do the fence bits though. > When Media pipeline is destroyed, callback for processing pixmap in GbmBuffer is also invalid. That seems to be the problem for crash in case we need to do any processing(Scaling/Format conversion) in GbmBuffer. Moving unsubmitted_frames_ to GbmSurfacless will fix it, if we handle any processing needs of the pixmap before adding it to the queue. We can still have it as it is (Here I mean), if we can ensure that any processing needs of the pixmap is handled before adding it to the queue.
On 2015/11/12 16:36:52, kalyank wrote: > https://codereview.chromium.org/1432963003/diff/20001/ui/gl/gl_surface_ozone.cc > File ui/gl/gl_surface_ozone.cc (right): > > https://codereview.chromium.org/1432963003/diff/20001/ui/gl/gl_surface_ozone.... > ui/gl/gl_surface_ozone.cc:227: bool > GLSurfaceOzoneSurfaceless::Overlay::ScheduleOverlayPlane( > On 2015/11/12 15:47:52, spang wrote: > > On 2015/11/12 15:37:43, william.xie wrote: > > > On 2015/11/12 14:58:26, reveman wrote: > > > > why is this called after the image has been destroyed? > > > > > > the image is in the unsubmitted_frames_, and will be scheduled to overlay > when > > > swapbuffer. During video playback on overlay, if user reload the webpage, > the > > > mediaplayer pipeline will be destroyed immediately, and the image will be > > > destroyed too. However, the image object itself is still valid as it is > cached > > > in pending frames, so this CL fixes the issue by checking if the image is > > still > > > valid before scheduling it to overlay. > > > > > > Just let me know if the explanation is not clear. > > > > If it was totally valid when scheduled, we can't just drop it on the floor > here. > > > > Probably unsubmitted_frames_ queuing logic should be moved to GbmSurfaceless. > > Need to find a way to do the fence bits though. > > > > When Media pipeline is destroyed, callback for processing pixmap in GbmBuffer is > also invalid. That seems to be the problem for crash in case we need to do any > processing(Scaling/Format conversion) in GbmBuffer. Moving unsubmitted_frames_ > to GbmSurfacless will fix it, if we handle any processing needs of the pixmap > before adding it to the queue. We can still have it as it is (Here I mean), if > we can ensure that any processing needs of the pixmap is handled before adding > it to the queue. Thanks so much, Kalyan, moving unsubmitted_frames_ to GbmSurfacless and will update the CL soon.
On 2015/11/12 16:43:44, william.xie wrote: > On 2015/11/12 16:36:52, kalyank wrote: > > > https://codereview.chromium.org/1432963003/diff/20001/ui/gl/gl_surface_ozone.cc > > File ui/gl/gl_surface_ozone.cc (right): > > > > > https://codereview.chromium.org/1432963003/diff/20001/ui/gl/gl_surface_ozone.... > > ui/gl/gl_surface_ozone.cc:227: bool > > GLSurfaceOzoneSurfaceless::Overlay::ScheduleOverlayPlane( > > On 2015/11/12 15:47:52, spang wrote: > > > On 2015/11/12 15:37:43, william.xie wrote: > > > > On 2015/11/12 14:58:26, reveman wrote: > > > > > why is this called after the image has been destroyed? > > > > > > > > the image is in the unsubmitted_frames_, and will be scheduled to overlay > > when > > > > swapbuffer. During video playback on overlay, if user reload the webpage, > > the > > > > mediaplayer pipeline will be destroyed immediately, and the image will be > > > > destroyed too. However, the image object itself is still valid as it is > > cached > > > > in pending frames, so this CL fixes the issue by checking if the image is > > > still > > > > valid before scheduling it to overlay. > > > > > > > > Just let me know if the explanation is not clear. > > > > > > If it was totally valid when scheduled, we can't just drop it on the floor > > here. > > > > > > Probably unsubmitted_frames_ queuing logic should be moved to > GbmSurfaceless. > > > Need to find a way to do the fence bits though. > > > > > > > When Media pipeline is destroyed, callback for processing pixmap in GbmBuffer > is > > also invalid. That seems to be the problem for crash in case we need to do any > > processing(Scaling/Format conversion) in GbmBuffer. Moving unsubmitted_frames_ > > to GbmSurfacless will fix it, if we handle any processing needs of the pixmap > > before adding it to the queue. We can still have it as it is (Here I mean), if > > we can ensure that any processing needs of the pixmap is handled before adding > > it to the queue. +1, it definitely sounds like we'll have to do the scaling up front while we have the context to do it. > Thanks so much, Kalyan, moving unsubmitted_frames_ to GbmSurfacless and will > update the CL soon.
On 2015/11/12 16:36:52, kalyank wrote: > https://codereview.chromium.org/1432963003/diff/20001/ui/gl/gl_surface_ozone.cc > File ui/gl/gl_surface_ozone.cc (right): > > https://codereview.chromium.org/1432963003/diff/20001/ui/gl/gl_surface_ozone.... > ui/gl/gl_surface_ozone.cc:227: bool > GLSurfaceOzoneSurfaceless::Overlay::ScheduleOverlayPlane( > On 2015/11/12 15:47:52, spang wrote: > > On 2015/11/12 15:37:43, william.xie wrote: > > > On 2015/11/12 14:58:26, reveman wrote: > > > > why is this called after the image has been destroyed? > > > > > > the image is in the unsubmitted_frames_, and will be scheduled to overlay > when > > > swapbuffer. During video playback on overlay, if user reload the webpage, > the > > > mediaplayer pipeline will be destroyed immediately, and the image will be > > > destroyed too. However, the image object itself is still valid as it is > cached > > > in pending frames, so this CL fixes the issue by checking if the image is > > still > > > valid before scheduling it to overlay. > > > > > > Just let me know if the explanation is not clear. > > > > If it was totally valid when scheduled, we can't just drop it on the floor > here. > > > > Probably unsubmitted_frames_ queuing logic should be moved to GbmSurfaceless. > > Need to find a way to do the fence bits though. > > > > When Media pipeline is destroyed, callback for processing pixmap in GbmBuffer is > also invalid. That seems to be the problem for crash in case we need to do any > processing(Scaling/Format conversion) in GbmBuffer. Moving unsubmitted_frames_ > to GbmSurfacless will fix it, if we handle any processing needs of the pixmap > before adding it to the queue. We can still have it as it is (Here I mean), if > we can ensure that any processing needs of the pixmap is handled before adding > it to the queue. It seems to me the core of the problem is that vaapi is destroyed pre-maturely. If a callback was created that relies on vaapi, that callback should hold a ref. In other words, vaapi cannot go away as long as there are callbacks referencing it. It's a lifecycle issue, maybe we should just fix that instead of re-arranging everything in gl_surface/platform. There is a lot of assumptions being made in gbm surfaceless about how buffers arrive, re-arranging that code is likely to introduce subtle bugs with front buffer rendering, which usually results in a lockup.
On 2015/11/12 18:12:38, alexst (slow to review) wrote: > On 2015/11/12 16:36:52, kalyank wrote: > > > https://codereview.chromium.org/1432963003/diff/20001/ui/gl/gl_surface_ozone.cc > > File ui/gl/gl_surface_ozone.cc (right): > > > > > https://codereview.chromium.org/1432963003/diff/20001/ui/gl/gl_surface_ozone.... > > ui/gl/gl_surface_ozone.cc:227: bool > > GLSurfaceOzoneSurfaceless::Overlay::ScheduleOverlayPlane( > > On 2015/11/12 15:47:52, spang wrote: > > > On 2015/11/12 15:37:43, william.xie wrote: > > > > On 2015/11/12 14:58:26, reveman wrote: > > > > > why is this called after the image has been destroyed? > > > > > > > > the image is in the unsubmitted_frames_, and will be scheduled to overlay > > when > > > > swapbuffer. During video playback on overlay, if user reload the webpage, > > the > > > > mediaplayer pipeline will be destroyed immediately, and the image will be > > > > destroyed too. However, the image object itself is still valid as it is > > cached > > > > in pending frames, so this CL fixes the issue by checking if the image is > > > still > > > > valid before scheduling it to overlay. > > > > > > > > Just let me know if the explanation is not clear. > > > > > > If it was totally valid when scheduled, we can't just drop it on the floor > > here. > > > > > > Probably unsubmitted_frames_ queuing logic should be moved to > GbmSurfaceless. > > > Need to find a way to do the fence bits though. > > > > > > > When Media pipeline is destroyed, callback for processing pixmap in GbmBuffer > is > > also invalid. That seems to be the problem for crash in case we need to do any > > processing(Scaling/Format conversion) in GbmBuffer. Moving unsubmitted_frames_ > > to GbmSurfacless will fix it, if we handle any processing needs of the pixmap > > before adding it to the queue. We can still have it as it is (Here I mean), if > > we can ensure that any processing needs of the pixmap is handled before adding > > it to the queue. > > It seems to me the core of the problem is that vaapi is destroyed pre-maturely. > If a callback was created that relies on vaapi, that callback should hold a ref. > In other words, vaapi cannot go away as long as there are callbacks referencing > it. It's a lifecycle issue, maybe we should just fix that instead of > re-arranging everything in gl_surface/platform. There is a lot of assumptions > being made in gbm surfaceless about how buffers arrive, re-arranging that code > is likely to introduce subtle bugs with front buffer rendering, which usually > results in a lockup. Even though, we don't have to re-arrange anything if we can ensure processing needs of pixmap are handled in GLSurfaceOzoneSurfaceless::ScheduleOverlayPlane, fixing the lifecycle issue seems more fool proof. +1
Would changing GLImageEGL::Destroy to not destroy the EGLImage help solve this issue? That could be done in the dtor instead as the EGLImage is not context specific.
On 2015/11/12 19:35:00, reveman wrote: > Would changing GLImageEGL::Destroy to not destroy the EGLImage help solve this > issue? That could be done in the dtor instead as the EGLImage is not context > specific. No it wouldn't. Issue here is that GbmPixmap can process native pixmap as needed (i.e. Scale it, format conversion etc). Users of NativePixmap can set callback, if they want to handle post processing of the pixmap. VaapiDrmPicture sets the callback in this case and is deleted when re-loading the webpage. In GbmBuffer we still expect the callback to be valid and try to use it to scale the pixmap. We can ensure in GbmPixmap to process the pixmap only if it's valid (Which we should probably in future), but than in Video case we will end up displaying video buffers in native size which might not match the resolution.
On 2015/11/12 19:57:47, kalyank wrote: > On 2015/11/12 19:35:00, reveman wrote: > > Would changing GLImageEGL::Destroy to not destroy the EGLImage help solve this > > issue? That could be done in the dtor instead as the EGLImage is not context > > specific. > Sorry please ignore my reply above. No it wouldn't. Issue here is that GbmPixmap can process native pixmap as needed (i.e. Scale it, format conversion etc). Users of NativePixmap can set callback, if they want to handle post processing of the pixmap. VaapiDrmPicture sets the callback in this case and is deleted when re-loading the webpage. In GbmPixmap we still expect the callback to be valid and try to use it to scale the pixmap. We can ensure in GbmPixmap to process the pixmap only if callback is valid (Which we should probably), but in Video case we will end up displaying video buffers in native size which might not match the resolution.
Dear Alex and all, Please be noted that patch Set 3 is NOT for upstream, just for your reference for discussion. As you see, we are trying to keep VaapiDrmPicture alive as long as the callback in pixmap by using scoped_refptr. From my testing, it does resolve our issue. However, this also introduces a big problem, that is memory leak problem because of circulating reference. VaapiDrmPicture keeps a reference of the pixmap, and now we are trying to keep a reference of VaapiDrmPicture in the callback of pixmap. hence these objects won't be released anymore. Actually, the pixmap in unsubmitted_frames_ are contained by VaapiDrmPicture, once VaapiDrmPicture is destroyed, we should also destroy pixmap too instead of scheduling them. Any concern if we drop them?
On 2015/11/13 10:25:12, william.xie wrote: > Dear Alex and all, > Any concern if we drop them? We cannot drop them, it will be very bad user experience. You will be scanning out primary plane but the frame will be missing Overlay content. It will be like one frame where everything is correct, than Video layer missing and than next frame where everything is normal (This would depend on the no of unsubmitted frames, from which Overlay plane updates will be skipped). This is something which will be noticeable to human eye immediately.
On 2015/11/13 18:06:57, kalyank wrote: > On 2015/11/13 10:25:12, william.xie wrote: > > Dear Alex and all, > > Any concern if we drop them? > > We cannot drop them, it will be very bad user experience. You will be scanning > out > primary plane but the frame will be missing Overlay content. It will be > like one frame where everything is correct, than Video layer missing and than > next > frame where everything is normal (This would depend on the no of unsubmitted > frames, > from which Overlay plane updates will be skipped). This is something which will > be > noticeable to human eye immediately. Thanks Kalyan. only one frame in unsubmitted is dropped. Since this is page reloading, actually I didn't notice that. but anyway, let's figure it out in a proper way by moving to GbmSurfaceless etc.
https://codereview.chromium.org/1432963003/diff/40001/content/common/gpu/medi... File content/common/gpu/media/vaapi_video_decode_accelerator.cc (right): https://codereview.chromium.org/1432963003/diff/40001/content/common/gpu/medi... content/common/gpu/media/vaapi_video_decode_accelerator.cc:933: pictures_.clear(); I have played around with this a bit and issue is that DrmPicture holds on to the refs of GLImage(As you mentioned) and pixmap_. We should release them when VaapiVideoDecodeAccelerator decides to clear the pictures. We could have ClearResources call in VaapiPicture which would be called by VaapiDecodeAccelerator before it clears the pictures cache. In SurfaceOzone, we would still have a ref to any in use GLImage(which has a ref to NativePixmap). So, the image and native pixmap will not be destroyed till SurfaceOzone is using them. In VaapiDrmPicture::ClearResources, we should do what ever we are doing currently in the destructor along with releasing references to gl_image_ and pixmap_. However, bit confusing thing is that we need to hold onto processed_pixmap_ here as VaapiDecodeAccelerator doesn't really manage it but GbmPixmap and GLImage doesn't have a ref to it.
posciak@chromium.org changed reviewers: + posciak@chromium.org
Please keep me in the loop for all changes to content/common/gpu/media. Thank you. https://codereview.chromium.org/1432963003/diff/40001/content/common/gpu/medi... File content/common/gpu/media/vaapi_drm_picture.cc (right): https://codereview.chromium.org/1432963003/diff/40001/content/common/gpu/medi... content/common/gpu/media/vaapi_drm_picture.cc:118: scoped_refptr<VaapiDrmPicture> ref = this; This shouldn't be required. You should be able to pass this directly to Bind. https://codereview.chromium.org/1432963003/diff/40001/content/common/gpu/medi... File content/common/gpu/media/vaapi_drm_picture.h (right): https://codereview.chromium.org/1432963003/diff/40001/content/common/gpu/medi... content/common/gpu/media/vaapi_drm_picture.h:33: VaapiDrmPicture(scoped_refptr<VaapiWrapper> vaapi_wrapper, const& https://codereview.chromium.org/1432963003/diff/40001/content/common/gpu/medi... File content/common/gpu/media/vaapi_jpeg_decode_accelerator.h (right): https://codereview.chromium.org/1432963003/diff/40001/content/common/gpu/medi... content/common/gpu/media/vaapi_jpeg_decode_accelerator.h:90: scoped_refptr<VaapiWrapper> vaapi_wrapper_; Is this change required? This class doesn't use VaapiDrmPicture. https://codereview.chromium.org/1432963003/diff/40001/content/common/gpu/medi... File content/common/gpu/media/vaapi_picture.h (right): https://codereview.chromium.org/1432963003/diff/40001/content/common/gpu/medi... content/common/gpu/media/vaapi_picture.h:30: public base::RefCounted<VaapiPicture> { This should likely be RefCountedThreadSafe. https://codereview.chromium.org/1432963003/diff/40001/content/common/gpu/medi... content/common/gpu/media/vaapi_picture.h:55: scoped_refptr<VaapiWrapper> vaapi_wrapper, const& https://codereview.chromium.org/1432963003/diff/40001/content/common/gpu/medi... File content/common/gpu/media/vaapi_tfp_picture.h (right): https://codereview.chromium.org/1432963003/diff/40001/content/common/gpu/medi... content/common/gpu/media/vaapi_tfp_picture.h:33: VaapiTFPPicture(scoped_refptr<VaapiWrapper> vaapi_wrapper, const& https://codereview.chromium.org/1432963003/diff/40001/content/common/gpu/medi... File content/common/gpu/media/vaapi_video_decode_accelerator.cc (right): https://codereview.chromium.org/1432963003/diff/40001/content/common/gpu/medi... content/common/gpu/media/vaapi_video_decode_accelerator.cc:933: pictures_.clear(); On 2015/11/14 10:42:06, kalyank wrote: > I have played around with this a bit and issue is that DrmPicture holds on to > the refs of GLImage(As you mentioned) and pixmap_. We should release them when > VaapiVideoDecodeAccelerator decides to clear the pictures. If there are any references to VaapiDrmPicture being held by ScalingCallback, we shouldn't be releasing them at this point. But we should be ok already, ~VaapiDrmPicture will free them when all references to it are dropped. > We could have > ClearResources call in VaapiPicture which would be called by > VaapiDecodeAccelerator before it clears the pictures cache. > In SurfaceOzone, we > would still have a ref to any in use GLImage(which has a ref to NativePixmap). > So, the image and native pixmap will not be destroyed till SurfaceOzone is using > them. > > In VaapiDrmPicture::ClearResources, we should do what ever we are doing > currently in the destructor along with releasing references to gl_image_ and > pixmap_. However, bit confusing thing is that we need to hold onto > processed_pixmap_ here as VaapiDecodeAccelerator doesn't really manage it but > GbmPixmap and GLImage doesn't have a ref to it. I'm confused why we'd need this. If I understand this CL correctly, this extends the lifetime of VaapiDrmPicture for it to stay alive while it's waiting for the ScalingCallback to execute. After that, the last reference to VaapiDrmPicture will be dropped and GLImage and Pixmap should be released in the destructor of VaapiDrmPicture. VaapiVideoDecodeAccelerator will not be holding any references to VaapiDrmPicture since we released them here and only VaapiDrmPicture holds references to GLImage and Pixmap, so VaapiDrmPicture should be responsible for releasing them (and it does so in ~VaapiDrmPicture). https://codereview.chromium.org/1432963003/diff/40001/content/common/gpu/medi... File content/common/gpu/media/vaapi_video_encode_accelerator.h (right): https://codereview.chromium.org/1432963003/diff/40001/content/common/gpu/medi... content/common/gpu/media/vaapi_video_encode_accelerator.h:153: scoped_refptr<VaapiWrapper> vaapi_wrapper_; Is this change needed?
Description was changed from ========== [Ozone] Ensure invalid pixmap is discarded When reloading webpage during video playback on overlay, VaapiDrmPicture is destroyed immediately, thus the pixmap holding VaapiDrmPicture is getting invalid. This CL ensure the invalid pixmap is discarded instead of being scheduled. BUG=554005 ========== to ========== [Ozone] Extends the lifetime of VaapiDrmPicture When reloading webpage during video playback on overlay, VaapiDrmPicture is destroyed immediately, thus the pixmap holding VaapiDrmPicture is getting invalid. This CL extends the lifetime of VaapiDrmPicture to ensure pixmaps being scheduled. BUG=554005 ==========
Description was changed from ========== [Ozone] Extends the lifetime of VaapiDrmPicture When reloading webpage during video playback on overlay, VaapiDrmPicture is destroyed immediately, thus the pixmap holding VaapiDrmPicture is getting invalid. This CL extends the lifetime of VaapiDrmPicture to ensure pixmaps being scheduled. BUG=554005 ========== to ========== [Ozone] Extends the lifetime of VaapiDrmPicture When reloading webpage during video playback on overlay, VaapiDrmPicture is destroyed immediately, thus the callback of processing pixmap is getting invalid. This CL extends the lifetime of VaapiDrmPicture to ensure pixmaps being scheduled. BUG=554005 ==========
Description was changed from ========== [Ozone] Extends the lifetime of VaapiDrmPicture When reloading webpage during video playback on overlay, VaapiDrmPicture is destroyed immediately, thus the callback of processing pixmap is getting invalid. This CL extends the lifetime of VaapiDrmPicture to ensure pixmaps being scheduled. BUG=554005 ========== to ========== [Ozone] Extends the lifetime of VaapiDrmPicture When reloading webpage during video playback on overlay, VaapiDrmPicture is destroyed immediately, thus the callback of processing pixmap is getting invalid. This CL extends the lifetime of VaapiDrmPicture to ensure pixmaps being scheduled. BUG=554005 ==========
Description was changed from ========== [Ozone] Extends the lifetime of VaapiDrmPicture When reloading webpage during video playback on overlay, VaapiDrmPicture is destroyed immediately, thus the callback of processing pixmap is getting invalid. This CL extends the lifetime of VaapiDrmPicture to ensure pixmaps being scheduled. BUG=554005 ========== to ========== [Ozone] Extends the lifetime of VaapiDrmPicture When reloading webpage during video playback on overlay, VaapiDrmPicture is destroyed immediately, thus the callback of processing pixmap is getting invalid. This CL extends the lifetime of VaapiDrmPicture to ensure pixmaps being processed. BUG=554005 ==========
Thanks Pawel, PTAL https://codereview.chromium.org/1432963003/diff/40001/content/common/gpu/medi... File content/common/gpu/media/vaapi_drm_picture.cc (right): https://codereview.chromium.org/1432963003/diff/40001/content/common/gpu/medi... content/common/gpu/media/vaapi_drm_picture.cc:118: scoped_refptr<VaapiDrmPicture> ref = this; On 2015/11/16 04:24:35, Pawel Osciak wrote: > This shouldn't be required. You should be able to pass this directly to Bind. Done. https://codereview.chromium.org/1432963003/diff/40001/content/common/gpu/medi... File content/common/gpu/media/vaapi_drm_picture.h (right): https://codereview.chromium.org/1432963003/diff/40001/content/common/gpu/medi... content/common/gpu/media/vaapi_drm_picture.h:33: VaapiDrmPicture(scoped_refptr<VaapiWrapper> vaapi_wrapper, On 2015/11/16 04:24:35, Pawel Osciak wrote: > const& Done. https://codereview.chromium.org/1432963003/diff/40001/content/common/gpu/medi... File content/common/gpu/media/vaapi_jpeg_decode_accelerator.h (right): https://codereview.chromium.org/1432963003/diff/40001/content/common/gpu/medi... content/common/gpu/media/vaapi_jpeg_decode_accelerator.h:90: scoped_refptr<VaapiWrapper> vaapi_wrapper_; On 2015/11/16 04:24:35, Pawel Osciak wrote: > Is this change required? This class doesn't use VaapiDrmPicture. Done. https://codereview.chromium.org/1432963003/diff/40001/content/common/gpu/medi... content/common/gpu/media/vaapi_jpeg_decode_accelerator.h:90: scoped_refptr<VaapiWrapper> vaapi_wrapper_; On 2015/11/16 04:24:35, Pawel Osciak wrote: > Is this change required? This class doesn't use VaapiDrmPicture. Dear Pawel, for compile error because of scoped_refptr<VaapiWrapper> Create https://codereview.chromium.org/1432963003/diff/40001/content/common/gpu/medi... File content/common/gpu/media/vaapi_picture.h (right): https://codereview.chromium.org/1432963003/diff/40001/content/common/gpu/medi... content/common/gpu/media/vaapi_picture.h:30: public base::RefCounted<VaapiPicture> { On 2015/11/16 04:24:35, Pawel Osciak wrote: > This should likely be RefCountedThreadSafe. Done. https://codereview.chromium.org/1432963003/diff/40001/content/common/gpu/medi... content/common/gpu/media/vaapi_picture.h:55: scoped_refptr<VaapiWrapper> vaapi_wrapper, On 2015/11/16 04:24:35, Pawel Osciak wrote: > const& Done. https://codereview.chromium.org/1432963003/diff/40001/content/common/gpu/medi... File content/common/gpu/media/vaapi_video_decode_accelerator.cc (right): https://codereview.chromium.org/1432963003/diff/40001/content/common/gpu/medi... content/common/gpu/media/vaapi_video_decode_accelerator.cc:933: pictures_.clear(); On 2015/11/14 10:42:06, kalyank wrote: > I have played around with this a bit and issue is that DrmPicture holds on to > the refs of GLImage(As you mentioned) and pixmap_. We should release them when > VaapiVideoDecodeAccelerator decides to clear the pictures. We could have > ClearResources call in VaapiPicture which would be called by > VaapiDecodeAccelerator before it clears the pictures cache. In SurfaceOzone, we > would still have a ref to any in use GLImage(which has a ref to NativePixmap). > So, the image and native pixmap will not be destroyed till SurfaceOzone is using > them. > > In VaapiDrmPicture::ClearResources, we should do what ever we are doing > currently in the destructor along with releasing references to gl_image_ and > pixmap_. However, bit confusing thing is that we need to hold onto > processed_pixmap_ here as VaapiDecodeAccelerator doesn't really manage it but > GbmPixmap and GLImage doesn't have a ref to it. Done. https://codereview.chromium.org/1432963003/diff/40001/content/common/gpu/medi... content/common/gpu/media/vaapi_video_decode_accelerator.cc:933: pictures_.clear(); On 2015/11/16 04:24:35, Pawel Osciak wrote: > On 2015/11/14 10:42:06, kalyank wrote: > > I have played around with this a bit and issue is that DrmPicture holds on to > > the refs of GLImage(As you mentioned) and pixmap_. We should release them when > > VaapiVideoDecodeAccelerator decides to clear the pictures. > > If there are any references to VaapiDrmPicture being held by ScalingCallback, we > shouldn't be releasing them at this point. But we should be ok already, > ~VaapiDrmPicture will free them when all references to it are dropped. > > > We could have > > ClearResources call in VaapiPicture which would be called by > > VaapiDecodeAccelerator before it clears the pictures cache. > > In SurfaceOzone, we > > would still have a ref to any in use GLImage(which has a ref to NativePixmap). > > So, the image and native pixmap will not be destroyed till SurfaceOzone is > using > > them. > > > > In VaapiDrmPicture::ClearResources, we should do what ever we are doing > > currently in the destructor along with releasing references to gl_image_ and > > pixmap_. However, bit confusing thing is that we need to hold onto > > processed_pixmap_ here as VaapiDecodeAccelerator doesn't really manage it but > > GbmPixmap and GLImage doesn't have a ref to it. > > I'm confused why we'd need this. If I understand this CL correctly, this extends > the lifetime of VaapiDrmPicture for it to stay alive while it's waiting for the > ScalingCallback to execute. After that, the last reference to VaapiDrmPicture > will be dropped and GLImage and Pixmap should be released in the destructor of > VaapiDrmPicture. > > VaapiVideoDecodeAccelerator will not be holding any references to > VaapiDrmPicture since we released them here and only VaapiDrmPicture holds > references to GLImage and Pixmap, so VaapiDrmPicture should be responsible for > releasing them (and it does so in ~VaapiDrmPicture). Done. https://codereview.chromium.org/1432963003/diff/40001/content/common/gpu/medi... File content/common/gpu/media/vaapi_video_encode_accelerator.h (right): https://codereview.chromium.org/1432963003/diff/40001/content/common/gpu/medi... content/common/gpu/media/vaapi_video_encode_accelerator.h:153: scoped_refptr<VaapiWrapper> vaapi_wrapper_; On 2015/11/16 04:24:35, Pawel Osciak wrote: > Is this change needed? Done. https://codereview.chromium.org/1432963003/diff/40001/content/common/gpu/medi... content/common/gpu/media/vaapi_video_encode_accelerator.h:153: scoped_refptr<VaapiWrapper> vaapi_wrapper_; On 2015/11/16 04:24:35, Pawel Osciak wrote: > Is this change needed? Dear Pawel, This change is for compile error as scoped_refptr<VaapiWrapper> Create...
william.xie@intel.com changed reviewers: + sean.v.kelley@intel.com
https://codereview.chromium.org/1432963003/diff/40001/content/common/gpu/medi... File content/common/gpu/media/vaapi_video_decode_accelerator.cc (right): https://codereview.chromium.org/1432963003/diff/40001/content/common/gpu/medi... content/common/gpu/media/vaapi_video_decode_accelerator.cc:933: pictures_.clear(); On 2015/11/16 04:24:35, Pawel Osciak wrote: > On 2015/11/14 10:42:06, kalyank wrote: > > I have played around with this a bit and issue is that DrmPicture holds on to > > the refs of GLImage(As you mentioned) and pixmap_. We should release them when > > VaapiVideoDecodeAccelerator decides to clear the pictures. > > If there are any references to VaapiDrmPicture being held by ScalingCallback, we > shouldn't be releasing them at this point. But we should be ok already, > ~VaapiDrmPicture will free them when all references to it are dropped. > > > We could have > > ClearResources call in VaapiPicture which would be called by > > VaapiDecodeAccelerator before it clears the pictures cache. > > In SurfaceOzone, we > > would still have a ref to any in use GLImage(which has a ref to NativePixmap). > > So, the image and native pixmap will not be destroyed till SurfaceOzone is > using > > them. > > > > In VaapiDrmPicture::ClearResources, we should do what ever we are doing > > currently in the destructor along with releasing references to gl_image_ and > > pixmap_. However, bit confusing thing is that we need to hold onto > > processed_pixmap_ here as VaapiDecodeAccelerator doesn't really manage it but > > GbmPixmap and GLImage doesn't have a ref to it. > > I'm confused why we'd need this. If I understand this CL correctly, this extends > the lifetime of VaapiDrmPicture for it to stay alive while it's waiting for the > ScalingCallback to execute. After that, the last reference to VaapiDrmPicture > will be dropped and GLImage and Pixmap should be released in the destructor of > VaapiDrmPicture. > > VaapiVideoDecodeAccelerator will not be holding any references to > VaapiDrmPicture since we released them here and only VaapiDrmPicture holds > references to GLImage and Pixmap, so VaapiDrmPicture should be responsible for > releasing them (and it does so in ~VaapiDrmPicture). We want to ensure VaapiDrmPicture is alive while any scaling callbacks are in use. For that, we changed callback to hold a ref of VaapiDrmPicture. Now: 1) VaapiVideoDecodeAccelerator and Callback has a reference to VaapiDrmPicture. 2)VaapiDrmPicture holds a reference to the GLImage and Native pixmap it creates. GLSurfaceOzone holds a reference to the same GLImage (In case its queued to be displayed). GLImage internally holds a reference to the Native Pixmap its initialized with. 3)GLSurfaceOzone will not hold any reference to GLImage, once it's done using it. However, GLImage is not destroyed as VaapiDrmPicture still holds a reference to it. 4)As GLImage is not destroyed, we don't release the NativePixmap it's initialized with. This results in keeping the Callback alive. Thus, GLImage, Native Pixmap and VaapiDrmPicture will never be destroyed. Releasing the resources in use by VaapiDrmPicture here, ensures that we clear all the resources once SurfaceOzone is done using the GLImage. I am not sure how easy is to separate the scaling pixmap logic out of DrmPicture and see if we can ensure only VaapiWrapper is alive (As it manages the VPP context).
On 2015/11/18 00:21:43, kalyank wrote: > https://codereview.chromium.org/1432963003/diff/40001/content/common/gpu/medi... > File content/common/gpu/media/vaapi_video_decode_accelerator.cc (right): > > https://codereview.chromium.org/1432963003/diff/40001/content/common/gpu/medi... > content/common/gpu/media/vaapi_video_decode_accelerator.cc:933: > pictures_.clear(); > On 2015/11/16 04:24:35, Pawel Osciak wrote: > > On 2015/11/14 10:42:06, kalyank wrote: > > > I have played around with this a bit and issue is that DrmPicture holds on > to > > > the refs of GLImage(As you mentioned) and pixmap_. We should release them > when > > > VaapiVideoDecodeAccelerator decides to clear the pictures. > > > > If there are any references to VaapiDrmPicture being held by ScalingCallback, > we > > shouldn't be releasing them at this point. But we should be ok already, > > ~VaapiDrmPicture will free them when all references to it are dropped. > > > > > We could have > > > ClearResources call in VaapiPicture which would be called by > > > VaapiDecodeAccelerator before it clears the pictures cache. > > > In SurfaceOzone, we > > > would still have a ref to any in use GLImage(which has a ref to > NativePixmap). > > > So, the image and native pixmap will not be destroyed till SurfaceOzone is > > using > > > them. > > > > > > In VaapiDrmPicture::ClearResources, we should do what ever we are doing > > > currently in the destructor along with releasing references to gl_image_ and > > > pixmap_. However, bit confusing thing is that we need to hold onto > > > processed_pixmap_ here as VaapiDecodeAccelerator doesn't really manage it > but > > > GbmPixmap and GLImage doesn't have a ref to it. > > > > I'm confused why we'd need this. If I understand this CL correctly, this > extends > > the lifetime of VaapiDrmPicture for it to stay alive while it's waiting for > the > > ScalingCallback to execute. After that, the last reference to VaapiDrmPicture > > will be dropped and GLImage and Pixmap should be released in the destructor of > > VaapiDrmPicture. > > > > VaapiVideoDecodeAccelerator will not be holding any references to > > VaapiDrmPicture since we released them here and only VaapiDrmPicture holds > > references to GLImage and Pixmap, so VaapiDrmPicture should be responsible for > > releasing them (and it does so in ~VaapiDrmPicture). > > We want to ensure VaapiDrmPicture is alive while any scaling callbacks are in > use. For that, we changed callback to hold a ref of VaapiDrmPicture. Now: > 1) VaapiVideoDecodeAccelerator and Callback has a reference to VaapiDrmPicture. > 2)VaapiDrmPicture holds a reference to the GLImage and Native pixmap it creates. > GLSurfaceOzone holds a reference to the same GLImage (In case its queued to be > displayed). GLImage internally holds a reference to the Native Pixmap its > initialized with. > 3)GLSurfaceOzone will not hold any reference to GLImage, once it's done using > it. However, GLImage is not destroyed as VaapiDrmPicture still holds a > reference to it. > 4)As GLImage is not destroyed, we don't release the NativePixmap it's > initialized with. This results in keeping the Callback alive. > > Thus, GLImage, Native Pixmap and VaapiDrmPicture will never be destroyed. > > Releasing the resources in use by VaapiDrmPicture here, ensures that we clear > all the resources once SurfaceOzone is done using the GLImage. > > I am not sure how easy is to separate the scaling pixmap logic out of DrmPicture > and see if we can ensure only VaapiWrapper is alive (As it manages the VPP > context). Thank you so much, Kalyan. Since VaapiDrmPicture also holds a scaled_pixmap_, we shouldn't kill VaapiDrmPicture when media pipeline is destroyed, so only keep VaapiWrapper alive won't work. I am trying to break the circulating reference by deleting pixmap/gl_image after they successfully been scanned out, for example in the callback function of pageflip. How do you think?
> I am trying to break the circulating reference by deleting pixmap/gl_image after > they successfully been scanned out, > for example in the callback function of pageflip. How do you think? If you mean callback in SurfaceOzone, wont we end with the same scenario of not releasing the resources at all ? Ensuring VaapiDrmPicture releases its GLImage and pixmap refs (All it's GL and Drm Resources) when Decoder is done using the pictures should break this.
Dear all, PTAL, We successfully break the circulating reference in this patch set.
https://codereview.chromium.org/1432963003/diff/100001/content/common/gpu/med... File content/common/gpu/media/vaapi_video_decode_accelerator.cc (right): https://codereview.chromium.org/1432963003/diff/100001/content/common/gpu/med... content/common/gpu/media/vaapi_video_decode_accelerator.cc:648: pictures_.clear(); We should be calling Destroy here too ?
PTAL
The CQ bit was checked by william.xie@intel.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/1432963003/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1432963003/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. Only full committers are accepted. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
https://codereview.chromium.org/1432963003/diff/120001/content/common/gpu/med... File content/common/gpu/media/vaapi_drm_picture.cc (right): https://codereview.chromium.org/1432963003/diff/120001/content/common/gpu/med... content/common/gpu/media/vaapi_drm_picture.cc:168: if (gl_image_ && make_context_current_.Run()) { Isn't pixmap_ required for the ProcessingCallback to do the conversion as well, and so can't be released here, before the callback executes? From what I can see, the circular dependency should be broken by not keeping VaapiDrmPicture in the callback, it's not needed for the callback anyway. We should instead use VaapiWrapper::BlitSurface, wrapping it into a static method that returns the processed pixmap, as the callback, which would only keep refs to the pixmaps and the wrapper, and we could release the VaapiDrmPicture as normal.
https://codereview.chromium.org/1432963003/diff/120001/content/common/gpu/med... File content/common/gpu/media/vaapi_drm_picture.cc (right): https://codereview.chromium.org/1432963003/diff/120001/content/common/gpu/med... content/common/gpu/media/vaapi_drm_picture.cc:168: if (gl_image_ && make_context_current_.Run()) { On 2015/11/19 08:03:45, Pawel Osciak wrote: > Isn't pixmap_ required for the ProcessingCallback to do the conversion as well, > and so can't be released here, before the callback executes? Here is just to release the ref of pixmap and the ref of gl_image, the actual pixmap and gl_image are not destroyed until callback is returned. > From what I can see, the circular dependency should be broken by not keeping > VaapiDrmPicture in the callback, it's not needed for the callback anyway. We > should instead use VaapiWrapper::BlitSurface, wrapping it into a static method > that returns the processed pixmap, as the callback, which would only keep refs > to the pixmaps and the wrapper, and we could release the VaapiDrmPicture as > normal. Dear Pawel, VaapiDrmPicture is required to keep alive because we need to create the processed pixmap in VaapiDrmPicture. otherwise, we have to wrap CreateNativePixmap to static method too. How do you think?
On 2015/11/19 08:03:46, Pawel Osciak wrote: > https://codereview.chromium.org/1432963003/diff/120001/content/common/gpu/med... > File content/common/gpu/media/vaapi_drm_picture.cc (right): > > https://codereview.chromium.org/1432963003/diff/120001/content/common/gpu/med... > content/common/gpu/media/vaapi_drm_picture.cc:168: if (gl_image_ && > make_context_current_.Run()) { > Isn't pixmap_ required for the ProcessingCallback to do the conversion as well, > and so can't be released here, before the callback executes? > > From what I can see, the circular dependency should be broken by not keeping > VaapiDrmPicture in the callback, it's not needed for the callback anyway. We > should instead use VaapiWrapper::BlitSurface, wrapping it into a static method > that returns the processed pixmap, as the callback, which would only keep refs > to the pixmaps and the wrapper, and we could release the VaapiDrmPicture as > normal.
https://codereview.chromium.org/1432963003/diff/120001/content/common/gpu/med... File content/common/gpu/media/vaapi_drm_picture.cc (right): https://codereview.chromium.org/1432963003/diff/120001/content/common/gpu/med... content/common/gpu/media/vaapi_drm_picture.cc:168: if (gl_image_ && make_context_current_.Run()) { On 2015/11/19 08:31:12, william.xie wrote: > On 2015/11/19 08:03:45, Pawel Osciak wrote: > > Isn't pixmap_ required for the ProcessingCallback to do the conversion as > well, > > and so can't be released here, before the callback executes? > Here is just to release the ref of pixmap and the ref of gl_image, the actual > pixmap and gl_image are not destroyed until callback is returned. > > > > > From what I can see, the circular dependency should be broken by not keeping > > VaapiDrmPicture in the callback, it's not needed for the callback anyway. We > > should instead use VaapiWrapper::BlitSurface, wrapping it into a static method > > that returns the processed pixmap, as the callback, which would only keep refs > > to the pixmaps and the wrapper, and we could release the VaapiDrmPicture as > > normal. > > Dear Pawel, VaapiDrmPicture is required to keep alive because we need to create > the processed pixmap in VaapiDrmPicture. otherwise, we have to wrap > CreateNativePixmap to static method too. How do you think? We should move VaapiDrmPicture::CreateVASurfaceForPixmap to VaapiWrapper. Then, we should have the caller of the callback create a native pixmap for the processing target and pass it to the callback. The callback should be static and call VaapiWrapper::CreateVASurfaceForPixmap for the pixmap given by the caller of the callback, and then call VaapiWrapper::BlitSurface on it.
On 2015/11/19 08:45:41, Pawel Osciak wrote: > https://codereview.chromium.org/1432963003/diff/120001/content/common/gpu/med... > File content/common/gpu/media/vaapi_drm_picture.cc (right): > > https://codereview.chromium.org/1432963003/diff/120001/content/common/gpu/med... > content/common/gpu/media/vaapi_drm_picture.cc:168: if (gl_image_ && > make_context_current_.Run()) { > On 2015/11/19 08:31:12, william.xie wrote: > > On 2015/11/19 08:03:45, Pawel Osciak wrote: > > > Isn't pixmap_ required for the ProcessingCallback to do the conversion as > > well, > > > and so can't be released here, before the callback executes? > > Here is just to release the ref of pixmap and the ref of gl_image, the actual > > pixmap and gl_image are not destroyed until callback is returned. > > > > > > > > > From what I can see, the circular dependency should be broken by not keeping > > > VaapiDrmPicture in the callback, it's not needed for the callback anyway. We > > > should instead use VaapiWrapper::BlitSurface, wrapping it into a static > method > > > that returns the processed pixmap, as the callback, which would only keep > refs > > > to the pixmaps and the wrapper, and we could release the VaapiDrmPicture as > > > normal. > > > > Dear Pawel, VaapiDrmPicture is required to keep alive because we need to > create > > the processed pixmap in VaapiDrmPicture. otherwise, we have to wrap > > CreateNativePixmap to static method too. How do you think? > > We should move VaapiDrmPicture::CreateVASurfaceForPixmap to VaapiWrapper. Then, > we should have the caller of the callback create a native pixmap for the > processing target and pass it to the callback. The callback should be static and > call VaapiWrapper::CreateVASurfaceForPixmap for the pixmap given by the caller > of the callback, and then call VaapiWrapper::BlitSurface on it. Good idea, changing it, thank you!
On 2015/11/19 08:51:29, william.xie wrote: > On 2015/11/19 08:45:41, Pawel Osciak wrote: > > > https://codereview.chromium.org/1432963003/diff/120001/content/common/gpu/med... > > File content/common/gpu/media/vaapi_drm_picture.cc (right): > > > > > https://codereview.chromium.org/1432963003/diff/120001/content/common/gpu/med... > > content/common/gpu/media/vaapi_drm_picture.cc:168: if (gl_image_ && > > make_context_current_.Run()) { > > On 2015/11/19 08:31:12, william.xie wrote: > > > On 2015/11/19 08:03:45, Pawel Osciak wrote: > > > > Isn't pixmap_ required for the ProcessingCallback to do the conversion as > > > well, > > > > and so can't be released here, before the callback executes? > > > Here is just to release the ref of pixmap and the ref of gl_image, the > actual > > > pixmap and gl_image are not destroyed until callback is returned. > > > > > > > > > > > > > From what I can see, the circular dependency should be broken by not > keeping > > > > VaapiDrmPicture in the callback, it's not needed for the callback anyway. > We > > > > should instead use VaapiWrapper::BlitSurface, wrapping it into a static > > method > > > > that returns the processed pixmap, as the callback, which would only keep > > refs > > > > to the pixmaps and the wrapper, and we could release the VaapiDrmPicture > as > > > > normal. > > > > > > Dear Pawel, VaapiDrmPicture is required to keep alive because we need to > > create > > > the processed pixmap in VaapiDrmPicture. otherwise, we have to wrap > > > CreateNativePixmap to static method too. How do you think? > > > > We should move VaapiDrmPicture::CreateVASurfaceForPixmap to VaapiWrapper. > Then, > > we should have the caller of the callback create a native pixmap for the > > processing target and pass it to the callback. The callback should be static > and > > call VaapiWrapper::CreateVASurfaceForPixmap for the pixmap given by the caller > > of the callback, and then call VaapiWrapper::BlitSurface on it. > > Good idea, changing it, thank you! Dear Pawel, I am trying to change VaapiWrapper::BlitSurface to static method, however, VaapiWrapper::BlitSurface require a few of members like va_display_ and va_vpp_context_id_ etc. there are two ways to fix it in my mind: 1: change VaapiWrapper to singleton 2: pass ref of VaapiWrapper to callback function Do you have any preference?
On 2015/11/19 10:10:56, william.xie wrote: > On 2015/11/19 08:51:29, william.xie wrote: > > On 2015/11/19 08:45:41, Pawel Osciak wrote: > > > > > > https://codereview.chromium.org/1432963003/diff/120001/content/common/gpu/med... > > > File content/common/gpu/media/vaapi_drm_picture.cc (right): > > > > > > > > > https://codereview.chromium.org/1432963003/diff/120001/content/common/gpu/med... > > > content/common/gpu/media/vaapi_drm_picture.cc:168: if (gl_image_ && > > > make_context_current_.Run()) { > > > On 2015/11/19 08:31:12, william.xie wrote: > > > > On 2015/11/19 08:03:45, Pawel Osciak wrote: > > > > > Isn't pixmap_ required for the ProcessingCallback to do the conversion > as > > > > well, > > > > > and so can't be released here, before the callback executes? > > > > Here is just to release the ref of pixmap and the ref of gl_image, the > > actual > > > > pixmap and gl_image are not destroyed until callback is returned. > > > > > > > > > > > > > > > > > From what I can see, the circular dependency should be broken by not > > keeping > > > > > VaapiDrmPicture in the callback, it's not needed for the callback > anyway. > > We > > > > > should instead use VaapiWrapper::BlitSurface, wrapping it into a static > > > method > > > > > that returns the processed pixmap, as the callback, which would only > keep > > > refs > > > > > to the pixmaps and the wrapper, and we could release the VaapiDrmPicture > > as > > > > > normal. > > > > > > > > Dear Pawel, VaapiDrmPicture is required to keep alive because we need to > > > create > > > > the processed pixmap in VaapiDrmPicture. otherwise, we have to wrap > > > > CreateNativePixmap to static method too. How do you think? > > > > > > We should move VaapiDrmPicture::CreateVASurfaceForPixmap to VaapiWrapper. > > Then, > > > we should have the caller of the callback create a native pixmap for the > > > processing target and pass it to the callback. The callback should be static > > and > > > call VaapiWrapper::CreateVASurfaceForPixmap for the pixmap given by the > caller > > > of the callback, and then call VaapiWrapper::BlitSurface on it. > > > > Good idea, changing it, thank you! > > Dear Pawel, I am trying to change VaapiWrapper::BlitSurface to static method, > however, > VaapiWrapper::BlitSurface require a few of members like va_display_ and > va_vpp_context_id_ etc. > there are two ways to fix it in my mind: > 1: change VaapiWrapper to singleton > 2: pass ref of VaapiWrapper to callback function > Do you have any preference? Sorry, I didn't mean to make VaapiWrapper::BlitSurface static, just the callback should be static, and call VaapiWrapper::CreateVASurfaceForPixmap for the pixmap given by the caller of the callback, and then call VaapiWrapper::BlitSurface on it. So yes, option 2, VaapiWrapper would be passed to that callback.
Description was changed from ========== [Ozone] Extends the lifetime of VaapiDrmPicture When reloading webpage during video playback on overlay, VaapiDrmPicture is destroyed immediately, thus the callback of processing pixmap is getting invalid. This CL extends the lifetime of VaapiDrmPicture to ensure pixmaps being processed. BUG=554005 ========== to ========== [Ozone] Extends the lifetime of VaapiWrapper Currently when reloading webpage during video playback on overlay, VaapiWrapper is destroyed immediately, thus the callback of processing pixmap is getting invalid. This CL extends the lifetime of VaapiWrapper to ensure pixmaps being processed. BUG=554005 ==========
Description was changed from ========== [Ozone] Extends the lifetime of VaapiWrapper Currently when reloading webpage during video playback on overlay, VaapiWrapper is destroyed immediately, thus the callback of processing pixmap is getting invalid. This CL extends the lifetime of VaapiWrapper to ensure pixmaps being processed. BUG=554005 ========== to ========== [Ozone] Extends the lifetime of VaapiWrapper Currently when reloading webpage during video playback on overlay, VaapiWrapper is destroyed immediately, thus the callback of processing pixmap is getting invalid. This CL extends the lifetime of VaapiWrapper to ensure pixmaps being processed. BUG=554005 ==========
Description was changed from ========== [Ozone] Extends the lifetime of VaapiWrapper Currently when reloading webpage during video playback on overlay, VaapiWrapper is destroyed immediately, thus the callback of processing pixmap is getting invalid. This CL extends the lifetime of VaapiWrapper to ensure pixmaps being processed. BUG=554005 ========== to ========== [Ozone] Extends the lifetime of VaapiWrapper Currently when reloading webpage during video playback on overlay, VaapiWrapper is destroyed immediately, thus the callback of processing pixmap is getting invalid. This CL extends the lifetime of VaapiWrapper to ensure pixmaps being processed. BUG=554005 ==========
Description was changed from ========== [Ozone] Extends the lifetime of VaapiWrapper Currently when reloading webpage during video playback on overlay, VaapiWrapper is destroyed immediately, thus the callback of processing pixmap is getting invalid. This CL extends the lifetime of VaapiWrapper to ensure pixmaps being processed. BUG=554005 ========== to ========== [Ozone] Extends the lifetime of VaapiWrapper Currently when reloading webpage during video playback on overlay, VaapiWrapper is destroyed immediately, thus the callback of processing pixmap is getting invalid. This CL extends the lifetime of VaapiWrapper to ensure pixmaps being processed. BUG=554005 ==========
Description was changed from ========== [Ozone] Extends the lifetime of VaapiWrapper Currently when reloading webpage during video playback on overlay, VaapiWrapper is destroyed immediately, thus the callback of processing pixmap is getting invalid. This CL extends the lifetime of VaapiWrapper to ensure pixmaps being processed. BUG=554005 ========== to ========== [Ozone] Extends the lifetime of VaapiWrapper Currently when reloading webpage during video playback on overlay, VaapiWrapper is destroyed immediately, thus the callback of processing pixmap is getting invalid. This CL extends the lifetime of VaapiWrapper to ensure pixmaps being processed. BUG=554005 ==========
Description was changed from ========== [Ozone] Extends the lifetime of VaapiWrapper Currently when reloading webpage during video playback on overlay, VaapiWrapper is destroyed immediately, thus the callback of processing pixmap is getting invalid. This CL extends the lifetime of VaapiWrapper to ensure pixmaps being processed. BUG=554005 ========== to ========== [Ozone] Extends the lifetime of VaapiWrapper Currently when reloading webpage during video playback on overlay, VaapiWrapper is destroyed immediately, thus the callback of processing pixmap is getting invalid. This CL extends the lifetime of VaapiWrapper to ensure pixmaps being processed. BUG=554005 ==========
@Pawel, @Spang, PTAL
https://chromiumcodereview.appspot.com/1432963003/diff/140001/content/common/... File content/common/gpu/media/vaapi_drm_picture.cc (right): https://chromiumcodereview.appspot.com/1432963003/diff/140001/content/common/... content/common/gpu/media/vaapi_drm_picture.cc:104: scoped_refptr<ui::NativePixmap> VaapiDrmPicture::ProcessPixmap( Could we have this in VaapiWrapper, and not static? https://chromiumcodereview.appspot.com/1432963003/diff/140001/content/common/... content/common/gpu/media/vaapi_drm_picture.cc:107: gfx::Size target_size, s/pixmap/source_pixmap/ https://chromiumcodereview.appspot.com/1432963003/diff/140001/content/common/... content/common/gpu/media/vaapi_drm_picture.cc:121: return nullptr; This is not needed, as va_surface will fall out of scope on return. Same for all other local variables in other return paths as well. https://chromiumcodereview.appspot.com/1432963003/diff/140001/content/common/... content/common/gpu/media/vaapi_drm_picture.cc:135: vaapi_wrapper->BlitSurface(va_surface, processed_va_surface); Perhaps: if (!vaapi_wrapper->BlitSurface(va_surface, processed_va_surface)) { } https://chromiumcodereview.appspot.com/1432963003/diff/140001/content/common/... File content/common/gpu/media/vaapi_drm_picture.h (right): https://chromiumcodereview.appspot.com/1432963003/diff/140001/content/common/... content/common/gpu/media/vaapi_drm_picture.h:53: static scoped_refptr<ui::NativePixmap> ProcessPixmap( Please document return value and |pixmap|. https://chromiumcodereview.appspot.com/1432963003/diff/140001/content/common/... File content/common/gpu/media/vaapi_wrapper.cc (right): https://chromiumcodereview.appspot.com/1432963003/diff/140001/content/common/... content/common/gpu/media/vaapi_wrapper.cc:163: return vaapi_wrapper.Pass(); Pass() should not be needed, here and below. https://chromiumcodereview.appspot.com/1432963003/diff/140001/content/common/... File content/common/gpu/media/vaapi_wrapper.h (right): https://chromiumcodereview.appspot.com/1432963003/diff/140001/content/common/... content/common/gpu/media/vaapi_wrapper.h:118: scoped_refptr<ui::NativePixmap> pixmap, const& https://chromiumcodereview.appspot.com/1432963003/diff/140001/content/common/... content/common/gpu/media/vaapi_wrapper.h:119: gfx::Size pixmap_size); const& https://chromiumcodereview.appspot.com/1432963003/diff/140001/content/common/... content/common/gpu/media/vaapi_wrapper.h:214: static uint32_t BufferFormatToVAFourCC(gfx::BufferFormat fmt); These methods don't need to be members of the class. Please just have them in anonymous namespace in .cc as they were in vaapi_drm_picture.cc. https://chromiumcodereview.appspot.com/1432963003/diff/140001/ui/ozone/platfo... File ui/ozone/platform/cast/surface_factory_cast.cc (right): https://chromiumcodereview.appspot.com/1432963003/diff/140001/ui/ozone/platfo... ui/ozone/platform/cast/surface_factory_cast.cc:196: gfx::Size size() override { return gfx::Size(); } I'm not sure this is what we'd like to have, i.e. reporting this as a 0x0 pixmap... https://chromiumcodereview.appspot.com/1432963003/diff/140001/ui/ozone/public... File ui/ozone/public/native_pixmap.h (right): https://chromiumcodereview.appspot.com/1432963003/diff/140001/ui/ozone/public... ui/ozone/public/native_pixmap.h:31: virtual gfx::Size size() = 0; This should be a const method. https://chromiumcodereview.appspot.com/1432963003/diff/140001/ui/ozone/public... ui/ozone/public/native_pixmap.h:55: gfx::Size, const&
PTAL https://codereview.chromium.org/1432963003/diff/140001/content/common/gpu/med... File content/common/gpu/media/vaapi_drm_picture.cc (right): https://codereview.chromium.org/1432963003/diff/140001/content/common/gpu/med... content/common/gpu/media/vaapi_drm_picture.cc:104: scoped_refptr<ui::NativePixmap> VaapiDrmPicture::ProcessPixmap( On 2015/11/20 10:09:18, Pawel Osciak wrote: > Could we have this in VaapiWrapper, and not static? Done. https://codereview.chromium.org/1432963003/diff/140001/content/common/gpu/med... content/common/gpu/media/vaapi_drm_picture.cc:107: gfx::Size target_size, On 2015/11/20 10:09:18, Pawel Osciak wrote: > s/pixmap/source_pixmap/ Done. https://codereview.chromium.org/1432963003/diff/140001/content/common/gpu/med... content/common/gpu/media/vaapi_drm_picture.cc:121: return nullptr; On 2015/11/20 10:09:18, Pawel Osciak wrote: > This is not needed, as va_surface will fall out of scope on return. > > Same for all other local variables in other return paths as well. Done. https://codereview.chromium.org/1432963003/diff/140001/content/common/gpu/med... content/common/gpu/media/vaapi_drm_picture.cc:135: vaapi_wrapper->BlitSurface(va_surface, processed_va_surface); On 2015/11/20 10:09:18, Pawel Osciak wrote: > Perhaps: > > if (!vaapi_wrapper->BlitSurface(va_surface, processed_va_surface)) { > } Done. https://codereview.chromium.org/1432963003/diff/140001/content/common/gpu/med... File content/common/gpu/media/vaapi_drm_picture.h (right): https://codereview.chromium.org/1432963003/diff/140001/content/common/gpu/med... content/common/gpu/media/vaapi_drm_picture.h:53: static scoped_refptr<ui::NativePixmap> ProcessPixmap( On 2015/11/20 10:09:18, Pawel Osciak wrote: > Please document return value and |pixmap|. Done. https://codereview.chromium.org/1432963003/diff/140001/content/common/gpu/med... File content/common/gpu/media/vaapi_wrapper.cc (right): https://codereview.chromium.org/1432963003/diff/140001/content/common/gpu/med... content/common/gpu/media/vaapi_wrapper.cc:163: return vaapi_wrapper.Pass(); On 2015/11/20 10:09:18, Pawel Osciak wrote: > Pass() should not be needed, here and below. Done. https://codereview.chromium.org/1432963003/diff/140001/content/common/gpu/med... File content/common/gpu/media/vaapi_wrapper.h (right): https://codereview.chromium.org/1432963003/diff/140001/content/common/gpu/med... content/common/gpu/media/vaapi_wrapper.h:118: scoped_refptr<ui::NativePixmap> pixmap, On 2015/11/20 10:09:18, Pawel Osciak wrote: > const& Done. https://codereview.chromium.org/1432963003/diff/140001/content/common/gpu/med... content/common/gpu/media/vaapi_wrapper.h:119: gfx::Size pixmap_size); On 2015/11/20 10:09:18, Pawel Osciak wrote: > const& Done. https://codereview.chromium.org/1432963003/diff/140001/content/common/gpu/med... content/common/gpu/media/vaapi_wrapper.h:214: static uint32_t BufferFormatToVAFourCC(gfx::BufferFormat fmt); On 2015/11/20 10:09:18, Pawel Osciak wrote: > These methods don't need to be members of the class. Please just have them in > anonymous namespace in .cc as they were in vaapi_drm_picture.cc. Done. https://codereview.chromium.org/1432963003/diff/140001/ui/ozone/platform/cast... File ui/ozone/platform/cast/surface_factory_cast.cc (right): https://codereview.chromium.org/1432963003/diff/140001/ui/ozone/platform/cast... ui/ozone/platform/cast/surface_factory_cast.cc:196: gfx::Size size() override { return gfx::Size(); } On 2015/11/20 10:09:18, Pawel Osciak wrote: > I'm not sure this is what we'd like to have, i.e. reporting this as a 0x0 > pixmap... @Spang, any comments? https://codereview.chromium.org/1432963003/diff/140001/ui/ozone/public/native... File ui/ozone/public/native_pixmap.h (right): https://codereview.chromium.org/1432963003/diff/140001/ui/ozone/public/native... ui/ozone/public/native_pixmap.h:31: virtual gfx::Size size() = 0; On 2015/11/20 10:09:18, Pawel Osciak wrote: > This should be a const method. Done. https://codereview.chromium.org/1432963003/diff/140001/ui/ozone/public/native... ui/ozone/public/native_pixmap.h:55: gfx::Size, On 2015/11/20 10:09:18, Pawel Osciak wrote: > const& Done.
lgtm
https://codereview.chromium.org/1432963003/diff/160001/content/common/gpu/med... File content/common/gpu/media/vaapi_wrapper.cc (right): https://codereview.chromium.org/1432963003/diff/160001/content/common/gpu/med... content/common/gpu/media/vaapi_wrapper.cc:313: CreateVASurfaceForPixmap(source_pixmap, source_pixmap->size()); Now, we create va surface of original pixmap, only to destroy it immediately. This doesn't look good. How expensive is this (i.e. Is this just a wrapper to dma_buf id here or will result in some copy etc) ? https://codereview.chromium.org/1432963003/diff/160001/content/common/gpu/med... File content/common/gpu/media/vaapi_wrapper.h (right): https://codereview.chromium.org/1432963003/diff/160001/content/common/gpu/med... content/common/gpu/media/vaapi_wrapper.h:87: ~VaapiWrapper(); This is refcounted, destructor should be made private/protected
PTAL https://codereview.chromium.org/1432963003/diff/160001/content/common/gpu/med... File content/common/gpu/media/vaapi_wrapper.cc (right): https://codereview.chromium.org/1432963003/diff/160001/content/common/gpu/med... content/common/gpu/media/vaapi_wrapper.cc:313: CreateVASurfaceForPixmap(source_pixmap, source_pixmap->size()); On 2015/11/23 17:51:07, kalyank wrote: > Now, we create va surface of original pixmap, only to destroy it immediately. > This doesn't look good. How expensive is this (i.e. Is this just a wrapper to > dma_buf id here or will result in some copy etc) ? Dear Kalyan, I measured the cost, it is quit light, less than 1ms, so it is just a wrapper to dma_buf, no copy. Regarding to destroying the pixmap immediately, libdrm has optimization for this usage. There is a cache list in libdrm to reuse the previous "freed" dma buffer objects as long as the size and format are matched. So the pixmaps are not freed by libdrm actually, but cached for next allocation. https://codereview.chromium.org/1432963003/diff/160001/content/common/gpu/med... File content/common/gpu/media/vaapi_wrapper.h (right): https://codereview.chromium.org/1432963003/diff/160001/content/common/gpu/med... content/common/gpu/media/vaapi_wrapper.h:87: ~VaapiWrapper(); On 2015/11/23 17:51:07, kalyank wrote: > This is refcounted, destructor should be made private/protected Done.
The CQ bit was checked by william.xie@intel.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/1432963003/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1432963003/180001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by william.xie@intel.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/1432963003/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1432963003/240001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #13 (id:240001) has been deleted
The CQ bit was checked by william.xie@intel.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/1432963003/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1432963003/260001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1432963003/diff/160001/content/common/gpu/med... File content/common/gpu/media/vaapi_wrapper.cc (right): https://codereview.chromium.org/1432963003/diff/160001/content/common/gpu/med... content/common/gpu/media/vaapi_wrapper.cc:313: CreateVASurfaceForPixmap(source_pixmap, source_pixmap->size()); On 2015/11/24 06:19:39, william.xie wrote: > On 2015/11/23 17:51:07, kalyank wrote: > > Now, we create va surface of original pixmap, only to destroy it immediately. > > This doesn't look good. How expensive is this (i.e. Is this just a wrapper to > > dma_buf id here or will result in some copy etc) ? > > Dear Kalyan, I measured the cost, it is quit light, less than 1ms, > so it is just a wrapper to dma_buf, no copy. > Regarding to destroying the pixmap immediately, libdrm has optimization for this > usage. There is a cache list in libdrm to reuse the previous "freed" dma buffer > objects as long as the size and format are matched. So the pixmaps are not freed > by libdrm actually, but cached for next allocation. MiniGBM doesn't use dri infrastructure internally and hence cannot use this. However, It should be ok if in media driver, va surface would internally just ref the dma_buf and not blit/copy from it. https://codereview.chromium.org/1432963003/diff/260001/content/common/gpu/med... File content/common/gpu/media/vaapi_wrapper.h (right): https://codereview.chromium.org/1432963003/diff/260001/content/common/gpu/med... content/common/gpu/media/vaapi_wrapper.h:227: friend class base::RefCountedThreadSafe<VaapiWrapper>; nit: should be at the end ? https://codereview.chromium.org/1432963003/diff/260001/ui/ozone/public/native... File ui/ozone/public/native_pixmap.h (right): https://codereview.chromium.org/1432963003/diff/260001/ui/ozone/public/native... ui/ozone/public/native_pixmap.h:31: virtual gfx::Size size() const = 0; GetBufferSize to be consistent with other API.
On 2015/11/24 23:00:25, kalyank wrote: > https://codereview.chromium.org/1432963003/diff/160001/content/common/gpu/med... > File content/common/gpu/media/vaapi_wrapper.cc (right): > > https://codereview.chromium.org/1432963003/diff/160001/content/common/gpu/med... > content/common/gpu/media/vaapi_wrapper.cc:313: > CreateVASurfaceForPixmap(source_pixmap, source_pixmap->size()); > On 2015/11/24 06:19:39, william.xie wrote: > > On 2015/11/23 17:51:07, kalyank wrote: > > > Now, we create va surface of original pixmap, only to destroy it > immediately. > > > This doesn't look good. How expensive is this (i.e. Is this just a wrapper > to > > > dma_buf id here or will result in some copy etc) ? > > > > Dear Kalyan, I measured the cost, it is quit light, less than 1ms, > > so it is just a wrapper to dma_buf, no copy. > > Regarding to destroying the pixmap immediately, libdrm has optimization for > this > > usage. There is a cache list in libdrm to reuse the previous "freed" dma > buffer > > objects as long as the size and format are matched. So the pixmaps are not > freed > > by libdrm actually, but cached for next allocation. > > MiniGBM doesn't use dri infrastructure internally and hence cannot use this. > However, It should be ok if in media driver, va surface would internally just > ref the dma_buf and not blit/copy from it. > > https://codereview.chromium.org/1432963003/diff/260001/content/common/gpu/med... > File content/common/gpu/media/vaapi_wrapper.h (right): > > https://codereview.chromium.org/1432963003/diff/260001/content/common/gpu/med... > content/common/gpu/media/vaapi_wrapper.h:227: friend class > base::RefCountedThreadSafe<VaapiWrapper>; > nit: should be at the end ? > > https://codereview.chromium.org/1432963003/diff/260001/ui/ozone/public/native... > File ui/ozone/public/native_pixmap.h (right): > > https://codereview.chromium.org/1432963003/diff/260001/ui/ozone/public/native... > ui/ozone/public/native_pixmap.h:31: virtual gfx::Size size() const = 0; > GetBufferSize to be consistent with other API. With the nits/API naming fixed non-owner LGTM
The CQ bit was checked by william.xie@intel.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/1432963003/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1432963003/280001
nits/API naming fix https://codereview.chromium.org/1432963003/diff/260001/content/common/gpu/med... File content/common/gpu/media/vaapi_wrapper.h (right): https://codereview.chromium.org/1432963003/diff/260001/content/common/gpu/med... content/common/gpu/media/vaapi_wrapper.h:227: friend class base::RefCountedThreadSafe<VaapiWrapper>; On 2015/11/24 23:00:24, kalyank wrote: > nit: should be at the end ? Done. https://codereview.chromium.org/1432963003/diff/260001/ui/ozone/public/native... File ui/ozone/public/native_pixmap.h (right): https://codereview.chromium.org/1432963003/diff/260001/ui/ozone/public/native... ui/ozone/public/native_pixmap.h:31: virtual gfx::Size size() const = 0; On 2015/11/24 23:00:24, kalyank wrote: > GetBufferSize to be consistent with other API. Done.
ping @Pawel
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://chromiumcodereview.appspot.com/1432963003/diff/280001/content/common/... File content/common/gpu/media/vaapi_wrapper.h (right): https://chromiumcodereview.appspot.com/1432963003/diff/280001/content/common/... content/common/gpu/media/vaapi_wrapper.h:130: scoped_refptr<ui::NativePixmap> CreateNativePixmap(gfx::Size size, Sorry, I think you misunderstood me previously: I only suggested having CreateVASurfaceForPixmap in VaapiWrapper, as it creates a VA surface from a pixmap. CreateNativePixmap() itself should not be an external API of VaapiWrapper, it's not related to VAAPI or a service that a VAAPI-specific class should be providing. We should be using SurfaceFactoryOzone to produce native pixmaps. Also, in my previous comment I suggested we should have the caller of the callback create the native pixmap for the processing target and pass it to the callback. The callback should be static and call VaapiWrapper::CreateVASurfaceForPixmap for the pixmap given by the caller of the callback, and then call VaapiWrapper::BlitSurface on it. Would that be possible please?
Dear Pawel, PTAL https://codereview.chromium.org/1432963003/diff/280001/content/common/gpu/med... File content/common/gpu/media/vaapi_wrapper.h (right): https://codereview.chromium.org/1432963003/diff/280001/content/common/gpu/med... content/common/gpu/media/vaapi_wrapper.h:130: scoped_refptr<ui::NativePixmap> CreateNativePixmap(gfx::Size size, On 2015/11/30 05:57:02, Pawel Osciak wrote: > Sorry, I think you misunderstood me previously: > > I only suggested having CreateVASurfaceForPixmap in VaapiWrapper, as it creates > a VA surface from a pixmap. CreateNativePixmap() itself should not be an > external API of VaapiWrapper, it's not related to VAAPI or a service that a > VAAPI-specific class should be providing. We should be using SurfaceFactoryOzone > to produce native pixmaps. > > Also, in my previous comment I suggested we should have the caller of the > callback create the native pixmap for the processing target and pass it to the > callback. The callback should be static and call > VaapiWrapper::CreateVASurfaceForPixmap for the pixmap given by the caller of the > callback, and then call VaapiWrapper::BlitSurface on it. > > Would that be possible please? Done.
The CQ bit was checked by william.xie@intel.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/1432963003/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1432963003/300001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by william.xie@intel.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/1432963003/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1432963003/320001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Softly ping Pawel@ :)
https://chromiumcodereview.appspot.com/1432963003/diff/320001/content/common/... File content/common/gpu/media/vaapi_wrapper.cc (right): https://chromiumcodereview.appspot.com/1432963003/diff/320001/content/common/... content/common/gpu/media/vaapi_wrapper.cc:692: vaapi_wrapper->CreateVASurfaceForPixmap(target_pixmap); Doesn't passing a raw pointer into a scoped_refptr in CreateVASurfaceForPixmap make that scoped_refptr destroy it at the end of CreateVASurfaceForPixmap? Perhaps we should consider passing a const refptr for target as well. https://chromiumcodereview.appspot.com/1432963003/diff/320001/content/common/... File content/common/gpu/media/vaapi_wrapper.h (right): https://chromiumcodereview.appspot.com/1432963003/diff/320001/content/common/... content/common/gpu/media/vaapi_wrapper.h:123: static bool ProcessPixmap( If we are making ProcessPixmap itself the callback, then there is no need to make it static. ProcessPixmap should not be static. Sorry if there was a confusion here. https://chromiumcodereview.appspot.com/1432963003/diff/320001/ui/ozone/public... File ui/ozone/public/native_pixmap.h (right): https://chromiumcodereview.appspot.com/1432963003/diff/320001/ui/ozone/public... ui/ozone/public/native_pixmap.h:51: // format. Please update this documentation.
https://chromiumcodereview.appspot.com/1432963003/diff/320001/content/common/... File content/common/gpu/media/vaapi_wrapper.cc (right): https://chromiumcodereview.appspot.com/1432963003/diff/320001/content/common/... content/common/gpu/media/vaapi_wrapper.cc:692: vaapi_wrapper->CreateVASurfaceForPixmap(target_pixmap); On 2015/12/02 00:49:06, Pawel Osciak wrote: > Doesn't passing a raw pointer into a scoped_refptr in CreateVASurfaceForPixmap > make that scoped_refptr destroy it at the end of CreateVASurfaceForPixmap? > > Perhaps we should consider passing a const refptr for target as well. Or, actually, just a non-const scoped_refptr.
https://chromiumcodereview.appspot.com/1432963003/diff/320001/content/common/... File content/common/gpu/media/vaapi_wrapper.cc (right): https://chromiumcodereview.appspot.com/1432963003/diff/320001/content/common/... content/common/gpu/media/vaapi_wrapper.cc:692: vaapi_wrapper->CreateVASurfaceForPixmap(target_pixmap); On 2015/12/02 02:08:34, Pawel Osciak wrote: > On 2015/12/02 00:49:06, Pawel Osciak wrote: > > Doesn't passing a raw pointer into a scoped_refptr in CreateVASurfaceForPixmap > > make that scoped_refptr destroy it at the end of CreateVASurfaceForPixmap? > > > > Perhaps we should consider passing a const refptr for target as well. > > Or, actually, just a non-const scoped_refptr. Dear Pawel, non-const scoped_refptr is not allowed, static check will fail. I am trying to use const scoped_refptr, but it looks strange. https://chromiumcodereview.appspot.com/1432963003/diff/320001/content/common/... File content/common/gpu/media/vaapi_wrapper.h (right): https://chromiumcodereview.appspot.com/1432963003/diff/320001/content/common/... content/common/gpu/media/vaapi_wrapper.h:123: static bool ProcessPixmap( On 2015/12/02 00:49:06, Pawel Osciak wrote: > If we are making ProcessPixmap itself the callback, then there is no need to > make it static. ProcessPixmap should not be static. Sorry if there was a > confusion here. Done.
https://chromiumcodereview.appspot.com/1432963003/diff/320001/content/common/... File content/common/gpu/media/vaapi_wrapper.cc (right): https://chromiumcodereview.appspot.com/1432963003/diff/320001/content/common/... content/common/gpu/media/vaapi_wrapper.cc:692: vaapi_wrapper->CreateVASurfaceForPixmap(target_pixmap); On 2015/12/02 02:15:26, william.xie wrote: > On 2015/12/02 02:08:34, Pawel Osciak wrote: > > On 2015/12/02 00:49:06, Pawel Osciak wrote: > > > Doesn't passing a raw pointer into a scoped_refptr in > CreateVASurfaceForPixmap > > > make that scoped_refptr destroy it at the end of CreateVASurfaceForPixmap? > > > > > > Perhaps we should consider passing a const refptr for target as well. > > > > Or, actually, just a non-const scoped_refptr. > > Dear Pawel, non-const scoped_refptr is not allowed, static check will fail. > I am trying to use const scoped_refptr, but it looks strange. Sorry, but passing a non-const scoped_refptr to a function is allowed and works. Could you point to the check that is failing please?
https://chromiumcodereview.appspot.com/1432963003/diff/320001/content/common/... File content/common/gpu/media/vaapi_wrapper.cc (right): https://chromiumcodereview.appspot.com/1432963003/diff/320001/content/common/... content/common/gpu/media/vaapi_wrapper.cc:692: vaapi_wrapper->CreateVASurfaceForPixmap(target_pixmap); On 2015/12/02 02:19:50, Pawel Osciak wrote: > On 2015/12/02 02:15:26, william.xie wrote: > > On 2015/12/02 02:08:34, Pawel Osciak wrote: > > > On 2015/12/02 00:49:06, Pawel Osciak wrote: > > > > Doesn't passing a raw pointer into a scoped_refptr in > > CreateVASurfaceForPixmap > > > > make that scoped_refptr destroy it at the end of CreateVASurfaceForPixmap? > > > > > > > > Perhaps we should consider passing a const refptr for target as well. > > > > > > Or, actually, just a non-const scoped_refptr. > > > > Dear Pawel, non-const scoped_refptr is not allowed, static check will fail. > > I am trying to use const scoped_refptr, but it looks strange. > > Sorry, but passing a non-const scoped_refptr to a function is allowed and works. > Could you point to the check that is failing please? I am sorry. non-const scoped_refptr to a function is allowed, I made a mistake by passing non-const scoped_refptr& to a function. The latest version is under testing, will upload soon. Thanks so much! https://chromiumcodereview.appspot.com/1432963003/diff/320001/ui/ozone/public... File ui/ozone/public/native_pixmap.h (right): https://chromiumcodereview.appspot.com/1432963003/diff/320001/ui/ozone/public... ui/ozone/public/native_pixmap.h:51: // format. On 2015/12/02 00:49:06, Pawel Osciak wrote: > Please update this documentation. Done.
Dear Pawel, here we go. PTAL
The CQ bit was checked by william.xie@intel.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/1432963003/340001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1432963003/340001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by william.xie@intel.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/1432963003/360001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1432963003/360001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1432963003/diff/360001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/gpu/gbm_buffer.cc (right): https://codereview.chromium.org/1432963003/diff/360001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/gbm_buffer.cc:108: if (!processing_callback_.Run(this, processed_pixmap.get())) { s/.get()// We need to pass the scoper, not a raw pointer here.
Sorry for the mistake. PTAL again. https://codereview.chromium.org/1432963003/diff/360001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/gpu/gbm_buffer.cc (right): https://codereview.chromium.org/1432963003/diff/360001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/gbm_buffer.cc:108: if (!processing_callback_.Run(this, processed_pixmap.get())) { On 2015/12/02 04:07:07, Pawel Osciak wrote: > s/.get()// > > We need to pass the scoper, not a raw pointer here. Done.
The CQ bit was checked by william.xie@intel.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/1432963003/380001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1432963003/380001
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by william.xie@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from spang@chromium.org, kalyan.kondapally@intel.com Link to the patchset: https://codereview.chromium.org/1432963003/#ps380001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1432963003/380001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1432963003/380001
Message was sent while issue was closed.
Description was changed from ========== [Ozone] Extends the lifetime of VaapiWrapper Currently when reloading webpage during video playback on overlay, VaapiWrapper is destroyed immediately, thus the callback of processing pixmap is getting invalid. This CL extends the lifetime of VaapiWrapper to ensure pixmaps being processed. BUG=554005 ========== to ========== [Ozone] Extends the lifetime of VaapiWrapper Currently when reloading webpage during video playback on overlay, VaapiWrapper is destroyed immediately, thus the callback of processing pixmap is getting invalid. This CL extends the lifetime of VaapiWrapper to ensure pixmaps being processed. BUG=554005 ==========
Message was sent while issue was closed.
Committed patchset #19 (id:380001)
Message was sent while issue was closed.
Description was changed from ========== [Ozone] Extends the lifetime of VaapiWrapper Currently when reloading webpage during video playback on overlay, VaapiWrapper is destroyed immediately, thus the callback of processing pixmap is getting invalid. This CL extends the lifetime of VaapiWrapper to ensure pixmaps being processed. BUG=554005 ========== to ========== [Ozone] Extends the lifetime of VaapiWrapper Currently when reloading webpage during video playback on overlay, VaapiWrapper is destroyed immediately, thus the callback of processing pixmap is getting invalid. This CL extends the lifetime of VaapiWrapper to ensure pixmaps being processed. BUG=554005 Committed: https://crrev.com/484a828e7021821546a6728eeba02792941a20ce Cr-Commit-Position: refs/heads/master@{#362650} ==========
Message was sent while issue was closed.
Patchset 19 (id:??) landed as https://crrev.com/484a828e7021821546a6728eeba02792941a20ce Cr-Commit-Position: refs/heads/master@{#362650}
Message was sent while issue was closed.
On 2015/12/02 06:46:09, commit-bot: I haz the power wrote: > Patchset 19 (id:??) landed as > https://crrev.com/484a828e7021821546a6728eeba02792941a20ce > Cr-Commit-Position: refs/heads/master@{#362650} changing scoped_ptr to scoped_refptr actually breaks compile of a unit tests: ../../content/common/gpu/media/vaapi_jpeg_decoder_unittest.cc:49:39: error: no member named 'reset' in 'scoped_refptr<content::VaapiWrapper>' void TearDown() override { wrapper_.reset(); } ~~~~~~~~ ^
Message was sent while issue was closed.
On 2015/12/11 16:27:44, binjin wrote: > On 2015/12/02 06:46:09, commit-bot: I haz the power wrote: > > Patchset 19 (id:??) landed as > > https://crrev.com/484a828e7021821546a6728eeba02792941a20ce > > Cr-Commit-Position: refs/heads/master@{#362650} > > changing scoped_ptr to scoped_refptr actually breaks compile of a unit tests: > > ../../content/common/gpu/media/vaapi_jpeg_decoder_unittest.cc:49:39: error: no > member named 'reset' in 'scoped_refptr<content::VaapiWrapper>' > void TearDown() override { wrapper_.reset(); } > ~~~~~~~~ ^ Thank you Binjin and sorry for that. I just submit a fix for it: https://codereview.chromium.org/1521823002/ |
