Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
490233002:
VaapiVideoAccelerator: make Vaapi accelerator work with ozone (Closed)
|
Created:
6 years, 4 months ago by llandwerlin-old Modified:
6 years ago Reviewers:
spang, vrk (LEFT CHROMIUM), jiajia.qin, reveman, gb.devel, seanvk, Owen Lin, vignatti (out of this project), dshwang, piman, jln (very slow on Chromium), marcheu, alexst (slow to review), kalyank, hshi1, Pawel Osciak, scherkus (not reviewing) CC:
chromium-reviews, rjkroege, ozone-reviews_chromium.org, posciak+watch_chromium.org, jam, mcasas+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, piman+watch_chromium.org, wjia+watch_chromium.org, jln+watch_chromium.org, Jorge Lucangeli Obes, leecam Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionVaapiVideoAccelerator: make Vaapi accelerator work with ozone
Refactor vaapi_wrapper to handle Ozone and X11 backends.
Tested on ChromiumOS by creating 2 images link/link_freon
BUG=403058
TEST=video_decode_accelerator_unittest and video_encode_accelerator_unittest
TBR=jln
Patch Set 1 #
Total comments: 34
Patch Set 2 : Update after reviews #Patch Set 3 : Update test field #
Total comments: 3
Patch Set 4 : Tentative unittests modifications #Patch Set 5 : Tentative unittests modifications 2 #Patch Set 6 : Unittests passing #Patch Set 7 : Reenable Ozone path #
Total comments: 6
Patch Set 8 : /Gbm/Drm/ #Patch Set 9 : Minimize changes to vaapi_wrapper.cc #Patch Set 10 : Don't limit h264_bitstream_buffer_unittest to x11 #
Total comments: 108
Patch Set 11 : Update after Pawel's second review #Patch Set 12 : Missing modifications in video accelerators #
Total comments: 60
Patch Set 13 : Wraps libVA calls into VaapiWrapper #Patch Set 14 : Remove unused members in VaapiPictureProviderDrm #Patch Set 15 : More VaapiPictureProviderDrm cleanup #
Total comments: 48
Patch Set 16 : Rebase on top of master #Patch Set 17 : Update after review #Patch Set 18 : Enable vaapi_h264_decoder_unittest on Ozone #
Total comments: 53
Patch Set 19 : Remove VaapiPictureProvider #Patch Set 20 : Remove stale comments about VaapiPictureProvider #Patch Set 21 : Removed commented code from VaapiWrapper #
Total comments: 12
Patch Set 22 : Fix GLX texture format enum #
Total comments: 2
Patch Set 23 : Use GLImageLinuxDMABuffer #Patch Set 24 : Update after review on RenderingHelper #
Total comments: 33
Patch Set 25 : RenderingHelper nits #Patch Set 26 : Update after Pawel's review #
Total comments: 5
Patch Set 27 : Remove picture destruction sequence #
Total comments: 47
Patch Set 28 : Update after Pawel's review #
Total comments: 11
Patch Set 29 : Rebase on ToT #
Total comments: 1
Patch Set 30 : Fix buffer exchange issue #Patch Set 31 : Fix vaapi_h264_decoder_unittest #
Total comments: 17
Patch Set 32 : scherkus' nits #Patch Set 33 : Rebase on ToT #
Total comments: 10
Messages
Total messages: 160 (16 generated)
https://codereview.chromium.org/490233002/diff/1/content/common/gpu/media/gpu... File content/common/gpu/media/gpu_video_decode_accelerator.cc (left): https://codereview.chromium.org/490233002/diff/1/content/common/gpu/media/gpu... content/common/gpu/media/gpu_video_decode_accelerator.cc:287: media::MediaOzonePlatform* platform = I would like to move the vaapi & v4l2 code out of content/common/gpu/media/, and let the platform construct the VDA it wants. So rather than removing this MediaOzonePlatform path, we should make it work properly on Intel. https://codereview.chromium.org/490233002/diff/1/content/common/gpu/media/gpu... File content/common/gpu/media/gpu_video_encode_accelerator.cc (right): https://codereview.chromium.org/490233002/diff/1/content/common/gpu/media/gpu... content/common/gpu/media/gpu_video_encode_accelerator.cc:193: const CommandLine* cmd_line = CommandLine::ForCurrentProcess(); no need to remove base:: https://codereview.chromium.org/490233002/diff/1/content/common/gpu/media/gpu... content/common/gpu/media/gpu_video_encode_accelerator.cc:195: encoder_.reset(new VaapiVideoEncodeAccelerator( We'll need to do something here to avoid hitting this path for non-drm based platforms. For example the buildbots running browser tests won't do direct hardware access. Letting the platform build the VDA fixes this in a natural way. https://codereview.chromium.org/490233002/diff/1/content/common/gpu/media/va_... File content/common/gpu/media/va_ozone.sigs (right): https://codereview.chromium.org/490233002/diff/1/content/common/gpu/media/va_... content/common/gpu/media/va_ozone.sigs:3: // found in the LICENSE file. this file should be va_drm.sigs, not va_ozone.sigs https://codereview.chromium.org/490233002/diff/1/content/common/gpu/media/vaa... File content/common/gpu/media/vaapi_video_decode_accelerator.h (right): https://codereview.chromium.org/490233002/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_video_decode_accelerator.h:150: // Display* x_display_; what's this commented out code? https://codereview.chromium.org/490233002/diff/1/content/content_common.gypi File content/content_common.gypi (right): https://codereview.chromium.org/490233002/diff/1/content/content_common.gypi#... content/content_common.gypi:787: 'common/gpu/media/va_ozone.sigs', call this va_drm.sigs
There is not really anything that VaapiWrapper uses from Backend apart from VADisplay (not counting Picture business, but that's should be separate). VADisplay is a proper abstraction already. Pictures, on the other hand, are an interface of VaapiVDA, not the wrapper. With this in mind: 1. Please have VaapiWrapper::Create factory method produce VADisplay based on platform. We can use that display without any calls to backend and independently from the platform. This one factory method is also all that is needed to abstract VaapiWrapper. 2. Please remove Backend from VaapiWrapper. The wrapper doesn't really need it, not any more than it needs make_context_current_ and gl_context. 3. Please don't require make_context_current_ and gl_context for the wrapper. They are specific to the Picture backend, not VaapiWrapper. Especially encoder doesn't need them. 4. Please move the Picture interface back to VaapiVideoDecodeAccelerator. This is a detail of that class, not VaapiWrapper, and no other class would use it. VaapiWrapper shouldn't implement it either. 5. Please turn Backends into Picture providers, i.e. Picture producers and converters, separate from VaapiWrapper and with platform-dependent implementations. X11 one would not need any connection to the Wrapper. The GBM one should have its own VaapiWrapper instance for vpp. It should also use it similarly to how VaapiVDA uses its own instance of VaapiWrapper. VaapiWrapper should manage GbmBackend's surfaces and be fully owned by GbmBackend, no need to refcount on each of the surfaces. They should be deleted with their VaapiWrapper instance solely owned by GbmBackend instance. 6. Please have VaapiVDA instantiate a picture provider (i.e. backend) from a platform-dependent factory and use it to produce and manage pictures. Thank you. https://codereview.chromium.org/490233002/diff/1/content/common/gpu/media/gpu... File content/common/gpu/media/gpu_video_decode_accelerator.cc (left): https://codereview.chromium.org/490233002/diff/1/content/common/gpu/media/gpu... content/common/gpu/media/gpu_video_decode_accelerator.cc:287: media::MediaOzonePlatform* platform = On 2014/08/22 17:20:23, spang wrote: > I would like to move the vaapi & v4l2 code out of content/common/gpu/media/, and > let the platform construct the VDA it wants. > Could you please explain why and how/to where would you like to move those VDAs? They are not really platform-dependent, only their display backend may be. And they don't care if we are ozone or not either... I agree we should abstract and move away perhaps the picture/display-related code that depends on the backend. But the VDAs should stay here, they are abstract enough. https://codereview.chromium.org/490233002/diff/1/content/common/gpu/media/gpu... File content/common/gpu/media/gpu_video_decode_accelerator.cc (right): https://codereview.chromium.org/490233002/diff/1/content/common/gpu/media/gpu... content/common/gpu/media/gpu_video_decode_accelerator.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. With TEST=None in CL description, how was this tested? You haven't modified video_decode_accelerator_unittest.cc. Please make it compile on ozone, fix it as needed and make sure it passes all test cases and that display from it works as well. Please also make sure that this works properly with dynamic resolution change. Also please make sure everything still works on X. https://codereview.chromium.org/490233002/diff/1/content/common/gpu/media/gpu... content/common/gpu/media/gpu_video_decode_accelerator.cc:275: #if defined(USE_X11) Do we need to make the X11 decision here? Could VaapiVDA fail initialization if its display backend fails to initialize if something is not supported? https://codereview.chromium.org/490233002/diff/1/content/common/gpu/media/gpu... content/common/gpu/media/gpu_video_decode_accelerator.cc:284: VLOG(1) << "HW video decode acceleration not available without " Please use DVLOG or DLOG https://codereview.chromium.org/490233002/diff/1/content/common/gpu/media/vaa... File content/common/gpu/media/vaapi_video_decode_accelerator.h (left): https://codereview.chromium.org/490233002/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_video_decode_accelerator.h:17: #include "base/memory/linked_ptr.h" linked_ptr is used in this file. Please keep this include. https://codereview.chromium.org/490233002/diff/1/content/common/gpu/media/vaa... File content/common/gpu/media/vaapi_video_decode_accelerator.h (right): https://codereview.chromium.org/490233002/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_video_decode_accelerator.h:150: // Display* x_display_; Please clean up all commented-out code in the whole CL. https://codereview.chromium.org/490233002/diff/1/content/common/gpu/media/vaa... File content/common/gpu/media/vaapi_wrapper.cc (left): https://codereview.chromium.org/490233002/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_wrapper.cc:168: va_display_ = vaGetDisplay(x_display); Could we keep va_display_ in VaapiWrapper? We wouldn't have to call this everywhere at each va* call site in VaapiWrapper. https://codereview.chromium.org/490233002/diff/1/content/common/gpu/media/vaa... File content/common/gpu/media/vaapi_wrapper.cc (right): https://codereview.chromium.org/490233002/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_wrapper.cc:11: #include "base/containers/scoped_ptr_hash_map.h" Unused? https://codereview.chromium.org/490233002/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_wrapper.cc:13: #include "base/memory/scoped_vector.h" Unused? https://codereview.chromium.org/490233002/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_wrapper.cc:67: class VaapiWrapper::Backend : public base::RefCounted<VaapiWrapper::Backend> { Please document all classes that are added. https://codereview.chromium.org/490233002/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_wrapper.cc:90: virtual void Deinitialize() {}; Does this need body? https://codereview.chromium.org/490233002/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_wrapper.cc:130: private: The proper order of members should start with public, protected and then private. Methods come first, then members. Please do not define methods inside class definitions. Also please add documentation. https://codereview.chromium.org/490233002/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_wrapper.cc:216: TFPPicture* tfp_picture = static_cast<TFPPicture*>(picture); Please do not cast to children. I think you can solve this by taking my suggestion and moving DestroyPicture to Picture implementations. https://codereview.chromium.org/490233002/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_wrapper.cc:257: va_display_ = vaGetDisplay(x_display_); Why not just call this once in the constructor? https://codereview.chromium.org/490233002/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_wrapper.cc:322: scoped_refptr<ui::NativePixmap> pixmap() const { return pixmap_; } Unused? https://codereview.chromium.org/490233002/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_wrapper.cc:405: 2); arraysize(va_attribs) https://codereview.chromium.org/490233002/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_wrapper.cc:414: gfx::GLImageEGL* gl_image = new gfx::GLImageEGL(size); Could we have a scoper for this instead? Where do we delete gl_image? I think we leak it? https://codereview.chromium.org/490233002/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_wrapper.cc:418: CHECK_EQ(static_cast<int>(glGetError()), GL_NO_ERROR); Please don't use CHECK. This crashes the process. Please handle the error if possible. https://codereview.chromium.org/490233002/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_wrapper.cc:464: GbmPicture* gbm_picture = static_cast<GbmPicture*>(picture); We cannot cast to child. But you don't really need anything here specific to GbmPicture, apart from gl_image(), or, actually, Bind(). If you instead had a Picture::Bind() virtual method, you could call it from this method, as well as from PutSurfaceIntoPicture in X11Backend. And you wouldn't have to cast here or there. https://codereview.chromium.org/490233002/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_wrapper.cc:554: DeinitializeVpp(); Isn't this called anyway on destruction via Deinitialize() ? https://codereview.chromium.org/490233002/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_wrapper.cc:1115: backend_->GetDisplay(), va_surface_id, picture); We don't need to pass to backend_ its own member. https://codereview.chromium.org/490233002/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_wrapper.cc:1116: VA_SUCCESS_OR_RETURN(va_res, "Failed putting surface to pixmap", false); s/pixmap/picture/? https://codereview.chromium.org/490233002/diff/1/content/common/gpu/media/vaa... File content/common/gpu/media/vaapi_wrapper.h (right): https://codereview.chromium.org/490233002/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_wrapper.h:51: class Picture : public base::NonThreadSafe { We should keep Picture as an interface in VaapiVideoDecodeAccelerator. There is nothing VAAPI-specific in it, it should not be in VaapiWrapper. It's also used solely by VaapiVideoDecodeAccelerator and is not a general interface for users of VaapiWrapper, because it contains VaapiVDA-specifics. https://codereview.chromium.org/490233002/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_wrapper.h:169: class X11Backend; All four declarations are unused here.
https://codereview.chromium.org/490233002/diff/1/content/common/gpu/media/gpu... File content/common/gpu/media/gpu_video_decode_accelerator.cc (left): https://codereview.chromium.org/490233002/diff/1/content/common/gpu/media/gpu... content/common/gpu/media/gpu_video_decode_accelerator.cc:287: media::MediaOzonePlatform* platform = On 2014/08/25 01:13:23, Pawel Osciak wrote: > On 2014/08/22 17:20:23, spang wrote: > > I would like to move the vaapi & v4l2 code out of content/common/gpu/media/, > and > > let the platform construct the VDA it wants. > > > > Could you please explain why and how/to where would you like to move those VDAs? > They are not really platform-dependent, only their display backend may be. And > they don't care if we are ozone or not either... I was thinking we should move it to media/ somewhere and let the platform instantiate it from media/ozone/. They are platform dependent, in that you may or may not have an implementation on your platform, as well as in their integration with the display. It seems cleaner to just have content ask for a working VDA object, and let the platform instantiate something appropriate. So that's the model I was proposing: move the implementations to media/, and give platforms control of building them. And if chromium porters somewhere have some other API than V4L2 or VA, they can use that to. > > I agree we should abstract and move away perhaps the picture/display-related > code that depends on the backend. But the VDAs should stay here, they are > abstract enough.
https://codereview.chromium.org/490233002/diff/1/content/common/gpu/media/vaa... File content/common/gpu/media/vaapi_wrapper.cc (right): https://codereview.chromium.org/490233002/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_wrapper.cc:478: pipeline_param->output_color_standard = VAProcColorStandardNone; isn't this going to differ from the X11 path? IIRC the X11 path uses BT601. Unless BT601 is the same as none of course :)
https://codereview.chromium.org/490233002/diff/1/content/common/gpu/media/gpu... File content/common/gpu/media/gpu_video_decode_accelerator.cc (left): https://codereview.chromium.org/490233002/diff/1/content/common/gpu/media/gpu... content/common/gpu/media/gpu_video_decode_accelerator.cc:287: media::MediaOzonePlatform* platform = On 2014/08/28 19:52:08, spang wrote: > On 2014/08/25 01:13:23, Pawel Osciak wrote: > > On 2014/08/22 17:20:23, spang wrote: > > > I would like to move the vaapi & v4l2 code out of content/common/gpu/media/, > > and > > > let the platform construct the VDA it wants. > > > > > > > Could you please explain why and how/to where would you like to move those > VDAs? > > They are not really platform-dependent, only their display backend may be. And > > they don't care if we are ozone or not either... > > I was thinking we should move it to media/ somewhere and let the platform > instantiate it from media/ozone/. > > They are platform dependent, in that you may or may not have an implementation > on your platform, as well as in their integration with the display. It seems > cleaner to just have content ask for a working VDA object, and let the platform > instantiate something appropriate. Agreed, but as you said, VDA impl choice is orthogonal to display backend choice. One VDA may be using different backends, and the choice criteria will not be the same either. I'd prefer if we could keep this one only about the backend choice for display for VaapiVDA please. I think that is complicated enough already :) > So that's the model I was proposing: move the implementations to media/, and > give platforms control of building them. And if chromium porters somewhere have > some other API than V4L2 or VA, they can use that to. Sure, this is worth discussing, but this would definitely be out of scope of this change :)
I tried to take into account most reviews. I forgot to mention this has been tested on ChromiumOS images (X11 and Ozone). It plays video properly on both backend. The X11 image passes successfully unit tests video_decode_accelerator_unittest and video_encode_accelerator_unittest and also running video_VideoSeek from autotest. I need to modify the unit tests a bit more to pass on Ozone. https://codereview.chromium.org/490233002/diff/1/content/common/gpu/media/vaa... File content/common/gpu/media/vaapi_wrapper.cc (right): https://codereview.chromium.org/490233002/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_wrapper.cc:478: pipeline_param->output_color_standard = VAProcColorStandardNone; On 2014/08/29 04:01:09, marcheu wrote: > isn't this going to differ from the X11 path? IIRC the X11 path uses BT601. > Unless BT601 is the same as none of course :) As far as I can tell the intel driver ignores this value and defaults to bt601. The nvidia driver falls back to BT601 if unsupported/not found.
On 2014/08/29 11:26:29, Pawel Osciak wrote: > https://codereview.chromium.org/490233002/diff/1/content/common/gpu/media/gpu... > File content/common/gpu/media/gpu_video_decode_accelerator.cc (left): > > https://codereview.chromium.org/490233002/diff/1/content/common/gpu/media/gpu... > content/common/gpu/media/gpu_video_decode_accelerator.cc:287: > media::MediaOzonePlatform* platform = > On 2014/08/28 19:52:08, spang wrote: > > On 2014/08/25 01:13:23, Pawel Osciak wrote: > > > On 2014/08/22 17:20:23, spang wrote: > > > > I would like to move the vaapi & v4l2 code out of > content/common/gpu/media/, > > > and > > > > let the platform construct the VDA it wants. > > > > > > > > > > Could you please explain why and how/to where would you like to move those > > VDAs? > > > They are not really platform-dependent, only their display backend may be. > And > > > they don't care if we are ozone or not either... > > > > I was thinking we should move it to media/ somewhere and let the platform > > instantiate it from media/ozone/. > > > > They are platform dependent, in that you may or may not have an implementation > > on your platform, as well as in their integration with the display. It seems > > cleaner to just have content ask for a working VDA object, and let the > platform > > instantiate something appropriate. > > Agreed, but as you said, VDA impl choice is orthogonal to display backend > choice. One VDA may be using different backends, and the choice criteria will > not be the same either. > > I'd prefer if we could keep this one only about the backend choice for display > for VaapiVDA please. I think that is complicated enough already :) Alright, as long as we deal with the correctness issues in another way. We need to disable everything related to gbm via gyp if ozone_platform_gbm=0, to avoid breaking buildbots that have old copies of gbm as well as other ports that don't build with gbm. And we also need to make sure the gbm path only activates if the gbm platform is selected *at runtime*. So ifdefs won't work here, ozone doesn't bind the platform until runtime and trying to do it at compile time could lead to bad casts, undefined behavior, etc. > > > So that's the model I was proposing: move the implementations to media/, and > > give platforms control of building them. And if chromium porters somewhere > have > > some other API than V4L2 or VA, they can use that to. > > Sure, this is worth discussing, but this would definitely be out of scope of > this change :)
https://codereview.chromium.org/490233002/diff/40001/content/common/gpu/media... File content/common/gpu/media/gpu_video_decode_accelerator.cc (left): https://codereview.chromium.org/490233002/diff/40001/content/common/gpu/media... content/common/gpu/media/gpu_video_decode_accelerator.cc:287: #elif defined(USE_OZONE) I'm concerned about removing this without providing an alternative. At least one external project is using it. We could maybe just do this for now: #elif defined(OS_CHROMEOS) && defined(ARCH_CPU_X86_FAMILY) // ChromeOS: use VA-API #elif defined(USE_OZONE) // !ChromeOS Ozone: Use MediaOzonePlatform
https://codereview.chromium.org/490233002/diff/40001/content/common/gpu/media... File content/common/gpu/media/gpu_video_decode_accelerator.cc (left): https://codereview.chromium.org/490233002/diff/40001/content/common/gpu/media... content/common/gpu/media/gpu_video_decode_accelerator.cc:287: #elif defined(USE_OZONE) On 2014/08/29 18:51:27, spang wrote: > I'm concerned about removing this without providing an alternative. At least one > external project is using it. > > We could maybe just do this for now: > > #elif defined(OS_CHROMEOS) && defined(ARCH_CPU_X86_FAMILY) > > // ChromeOS: use VA-API > > #elif defined(USE_OZONE) > > // !ChromeOS Ozone: Use MediaOzonePlatform Ok, I wasn't aware there was another project out there using the Ozone abstraction. So far I didn't see any implementation of the Ozone interface and it seems like an abstraction of this function.
tiago.vignatti@intel.com changed reviewers: + tiago.vignatti@intel.com
https://codereview.chromium.org/490233002/diff/40001/content/common/gpu/media... File content/common/gpu/media/gpu_video_decode_accelerator.cc (left): https://codereview.chromium.org/490233002/diff/40001/content/common/gpu/media... content/common/gpu/media/gpu_video_decode_accelerator.cc:287: #elif defined(USE_OZONE) On 2014/09/01 09:39:13, llandwerlin wrote: > On 2014/08/29 18:51:27, spang wrote: > > I'm concerned about removing this without providing an alternative. At least > one > > external project is using it. > > > > We could maybe just do this for now: > > > > #elif defined(OS_CHROMEOS) && defined(ARCH_CPU_X86_FAMILY) > > > > // ChromeOS: use VA-API > > > > #elif defined(USE_OZONE) > > > > // !ChromeOS Ozone: Use MediaOzonePlatform > > Ok, I wasn't aware there was another project out there using the Ozone > abstraction. So far I didn't see any implementation of the Ozone interface and > it seems like an abstraction of this function. yeah, Ozone-Wayland is using it at the moment: https://github.com/01org/ozone-wayland/tree/master/media and although I have not been active on the (internal) Ozone-GBM CL, I was planning to use the same interface for it: https://codereview.chromium.org/385793002/
Is there any documentation/example showing how to initialize a GL context on Ozone-Gbm? I'm struggling to get the test working. Depending on how I initialize things, I either get a crash in Mesa when calling SwapBuffers() or Mesa calling exit() when the kernel notifies it one of the buffer submitted isn't valid.
On 2014/09/02 16:47:55, llandwerlin wrote: > Is there any documentation/example showing how to initialize a GL context on > Ozone-Gbm? > > I'm struggling to get the test working. Depending on how I initialize things, I > either get a crash in Mesa when calling SwapBuffers() or Mesa calling exit() > when the kernel notifies it one of the buffer submitted isn't valid. There is a minimal example in the tree called ozone_demo (in ui/ozone/demo). Shouldn't need any particular flags. If you build it in the link_freon sdk (cros chrome-sdk --board=link_freon; ninja -C out_link_freon/Release ozone_demo) and run it on pixel, it will work (although not fullscreen).
On 2014/09/02 17:32:04, spang wrote: > On 2014/09/02 16:47:55, llandwerlin wrote: > > Is there any documentation/example showing how to initialize a GL context on > > Ozone-Gbm? > > > > I'm struggling to get the test working. Depending on how I initialize things, > I > > either get a crash in Mesa when calling SwapBuffers() or Mesa calling exit() > > when the kernel notifies it one of the buffer submitted isn't valid. > > There is a minimal example in the tree called ozone_demo (in ui/ozone/demo). > Shouldn't need any particular flags. > > If you build it in the link_freon sdk (cros chrome-sdk --board=link_freon; ninja > -C out_link_freon/Release ozone_demo) and run it on pixel, it will work > (although not fullscreen). That does work, unless you pass --ozone-platform=gbm. And without gbm you don't get GL.
On 2014/09/02 16:47:55, llandwerlin wrote: > Is there any documentation/example showing how to initialize a GL context on > Ozone-Gbm? > > I'm struggling to get the test working. Depending on how I initialize things, I > either get a crash in Mesa when calling SwapBuffers() or Mesa calling exit() > when the kernel notifies it one of the buffer submitted isn't valid. Oh dear, it looks like we broke the gbm platform when there's no gpu process. I guess we'll have to fix that.
On 2014/09/02 18:05:30, spang wrote: > On 2014/09/02 16:47:55, llandwerlin wrote: > > Is there any documentation/example showing how to initialize a GL context on > > Ozone-Gbm? > > > > I'm struggling to get the test working. Depending on how I initialize things, > I > > either get a crash in Mesa when calling SwapBuffers() or Mesa calling exit() > > when the kernel notifies it one of the buffer submitted isn't valid. > > Oh dear, it looks like we broke the gbm platform when there's no gpu process. > > I guess we'll have to fix that. Can you try again as of aa2a372038d7faf53f0f98194fdac467b56580de ?
On 2014/09/03 15:49:01, spang wrote: > > Can you try again as of aa2a372038d7faf53f0f98194fdac467b56580de ? Sure, I spotted your commit this morning and tried it. Now I have to work around a bug in Mesa it seems. It seems I can't do 2 glSwapBuffers() without a glClear() in between. Also I can't see any video on the screen. Would you like me to push the modification to the test program to have a look?
On 2014/09/03 15:55:05, llandwerlin wrote: > On 2014/09/03 15:49:01, spang wrote: > > > > > Can you try again as of aa2a372038d7faf53f0f98194fdac467b56580de ? > > > Sure, I spotted your commit this morning and tried it. > Now I have to work around a bug in Mesa it seems. > > It seems I can't do 2 glSwapBuffers() without a glClear() in between. > Also I can't see any video on the screen. > > Would you like me to push the modification to the test program to have a look? Sure.
Wrong commit sorry...
I figured out the problem with the screen_size_ not being set (not much to see on a 0x0 viewport...). Most tests pass except the thumbnail one. I haven't figured out what's wrong with the texture : https://imgur.com/eBVeybk That doesn't happen inside Chrome.
PTAL Pawel: I left the VPP stuff in the picture provider for now. I would like to move it to vaapi_wrapper in another CL to use it to convert I420 to NV12 in the encoder, if that's ok. Regarding the test-25fps.h264.md5 checksum changes, libva uses 2 different paths for color conversion under X11 and without of X11. It turns out the X11 shader used for conversion can be parameterized with coefficients, whereas the VPP shader has hardcoded BT709 coefficients. This is obviously something we need to fix in libva, but maybe this can be done in another CL. Unfortunately I didn't manage to run the link_freon image on a SandyBridge machine, so I don't have the checksum for it yet. Do you know if other images than link work with the ozone backend? Last thing, when running the tests, if the first test is run, the second will often fail with Mesa calling exit() because the kernel reports an invalid buffer object has been used. If you run all the test but the first one, they all pass. More recent kernel versions can actually tell what buffer object is invalid, so I'll try that to figure out what's wrong.
This needs to avoid assuming gbm in common code for the reasons I mentioned earlier. The only files that can assume gbm are the ones at ui/ozone/platform/dri/*gbm* To fix this, just call into gbm indirectly via the interfaces in ui/ozone/public - SurfaceFactoryOzone & NativePixmap. https://codereview.chromium.org/490233002/diff/120001/content/common/gpu/medi... File content/common/gpu/media/vaapi_picture_provider.cc (right): https://codereview.chromium.org/490233002/diff/120001/content/common/gpu/medi... content/common/gpu/media/vaapi_picture_provider.cc:15: #include <gbm.h> Remove gbm #include. https://codereview.chromium.org/490233002/diff/120001/content/common/gpu/medi... content/common/gpu/media/vaapi_picture_provider.cc:254: class GbmVaapiPictureProvider : public VaapiPictureProvider { s/Gbm/Drm/ https://codereview.chromium.org/490233002/diff/120001/content/common/gpu/medi... content/common/gpu/media/vaapi_picture_provider.cc:294: class GbmPicture : public VaapiPictureProvider::Picture { s/Gbm/Drm/ https://codereview.chromium.org/490233002/diff/120001/content/common/gpu/medi... content/common/gpu/media/vaapi_picture_provider.cc:355: va_attrib_extbuf.data_size = size.height() * gbm_bo_get_stride (bo); Move these gbm calls into GbmPixmap & add a virtual function to NativePixmap to get the stride. https://codereview.chromium.org/490233002/diff/120001/content/common/gpu/medi... File content/common/gpu/media/vaapi_wrapper.cc (right): https://codereview.chromium.org/490233002/diff/120001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.cc:20: #include <gbm.h> Remove gbm #include. https://codereview.chromium.org/490233002/diff/120001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.cc:158: Add a SurfaceFactoryOzone::GetDrmFd() and implement it in dri_surface_factory.cc. Use this to replace the call to gbm_device_get_fd.
On 2014/09/05 10:36:36, llandwerlin wrote: > PTAL > > Pawel: I left the VPP stuff in the picture provider for now. I would like to > move it to vaapi_wrapper in another CL to use it to convert I420 to NV12 in the > encoder, if that's ok. > > Regarding the test-25fps.h264.md5 checksum changes, libva uses 2 different paths > for color conversion under X11 and without of X11. It turns out the X11 shader > used for conversion can be parameterized with coefficients, whereas the VPP > shader has hardcoded BT709 coefficients. This is obviously something we need to > fix in libva... Keep in mind there is no VEBOX on SNB, Ivy or Baytrail. At the same time when using the VEBOX to do the CSC conversion from say NV12 to RGB or vice versa, it doesn't handle the scenarios where input/output have different resolutions. But the shader can handle it. So we disable it in the VEBOX pipeline. The reason to disable IECP on VEBOX is that it is more flexible to support CSC for multiple pixel formats by shader. Then if you need scaling, etc, you need more than one pipeline. Shaders are just more flexible. So please speak with me before you make assumptions about VPP... Lionel, we also need to touch base on these changes you are submitting as I need run these against current enabling work for VP8 encode. Sean
On 2014/09/05 20:45:46, spang wrote: > This needs to avoid assuming gbm in common code for the reasons I mentioned > earlier. > > The only files that can assume gbm are the ones at ui/ozone/platform/dri/*gbm* > > To fix this, just call into gbm indirectly via the interfaces in ui/ozone/public > - SurfaceFactoryOzone & NativePixmap. > > https://codereview.chromium.org/490233002/diff/120001/content/common/gpu/medi... > File content/common/gpu/media/vaapi_picture_provider.cc (right): > > https://codereview.chromium.org/490233002/diff/120001/content/common/gpu/medi... > content/common/gpu/media/vaapi_picture_provider.cc:15: #include <gbm.h> > Remove gbm #include. > > https://codereview.chromium.org/490233002/diff/120001/content/common/gpu/medi... > content/common/gpu/media/vaapi_picture_provider.cc:254: class > GbmVaapiPictureProvider : public VaapiPictureProvider { > s/Gbm/Drm/ > > https://codereview.chromium.org/490233002/diff/120001/content/common/gpu/medi... > content/common/gpu/media/vaapi_picture_provider.cc:294: class GbmPicture : > public VaapiPictureProvider::Picture { > s/Gbm/Drm/ > > https://codereview.chromium.org/490233002/diff/120001/content/common/gpu/medi... > content/common/gpu/media/vaapi_picture_provider.cc:355: > va_attrib_extbuf.data_size = size.height() * gbm_bo_get_stride (bo); > Move these gbm calls into GbmPixmap & add a virtual function to NativePixmap to > get the stride. > > https://codereview.chromium.org/490233002/diff/120001/content/common/gpu/medi... > File content/common/gpu/media/vaapi_wrapper.cc (right): > > https://codereview.chromium.org/490233002/diff/120001/content/common/gpu/medi... > content/common/gpu/media/vaapi_wrapper.cc:20: #include <gbm.h> > Remove gbm #include. > > https://codereview.chromium.org/490233002/diff/120001/content/common/gpu/medi... > content/common/gpu/media/vaapi_wrapper.cc:158: > Add a SurfaceFactoryOzone::GetDrmFd() and implement it in > dri_surface_factory.cc. Use this to replace the call to gbm_device_get_fd. Hey there, I integrated the modifications you requested, let me know if there is anything else I need to change. Thanks.
On 2014/09/08 17:17:08, llandwerlin wrote: > On 2014/09/05 20:45:46, spang wrote: > > This needs to avoid assuming gbm in common code for the reasons I mentioned > > earlier. > > > > The only files that can assume gbm are the ones at ui/ozone/platform/dri/*gbm* > > > > To fix this, just call into gbm indirectly via the interfaces in > ui/ozone/public > > - SurfaceFactoryOzone & NativePixmap. > > > > > https://codereview.chromium.org/490233002/diff/120001/content/common/gpu/medi... > > File content/common/gpu/media/vaapi_picture_provider.cc (right): > > > > > https://codereview.chromium.org/490233002/diff/120001/content/common/gpu/medi... > > content/common/gpu/media/vaapi_picture_provider.cc:15: #include <gbm.h> > > Remove gbm #include. > > > > > https://codereview.chromium.org/490233002/diff/120001/content/common/gpu/medi... > > content/common/gpu/media/vaapi_picture_provider.cc:254: class > > GbmVaapiPictureProvider : public VaapiPictureProvider { > > s/Gbm/Drm/ > > > > > https://codereview.chromium.org/490233002/diff/120001/content/common/gpu/medi... > > content/common/gpu/media/vaapi_picture_provider.cc:294: class GbmPicture : > > public VaapiPictureProvider::Picture { > > s/Gbm/Drm/ > > > > > https://codereview.chromium.org/490233002/diff/120001/content/common/gpu/medi... > > content/common/gpu/media/vaapi_picture_provider.cc:355: > > va_attrib_extbuf.data_size = size.height() * gbm_bo_get_stride (bo); > > Move these gbm calls into GbmPixmap & add a virtual function to NativePixmap > to > > get the stride. > > > > > https://codereview.chromium.org/490233002/diff/120001/content/common/gpu/medi... > > File content/common/gpu/media/vaapi_wrapper.cc (right): > > > > > https://codereview.chromium.org/490233002/diff/120001/content/common/gpu/medi... > > content/common/gpu/media/vaapi_wrapper.cc:20: #include <gbm.h> > > Remove gbm #include. > > > > > https://codereview.chromium.org/490233002/diff/120001/content/common/gpu/medi... > > content/common/gpu/media/vaapi_wrapper.cc:158: > > Add a SurfaceFactoryOzone::GetDrmFd() and implement it in > > dri_surface_factory.cc. Use this to replace the call to gbm_device_get_fd. > > Hey there, > > I integrated the modifications you requested, let me know if there is anything > else I need to change. > > Thanks. ui/ozone lgtm
lionel.g.landwerlin@intel.com changed reviewers: + cevans@chromium.org, vrk@chromium.org
cevans@chromium.org: Please review changes in the content/common/sandbox_linux/bpf_gpu_policy_linux.cc and content/common/BUILD.gn vrk@chromium.org: Please review changes in media/test Thank you
lionel.g.landwerlin@intel.com changed reviewers: - cevans@chromium.org
lionel.g.landwerlin@intel.com changed reviewers: + jln@chromium.org
jln@chromium.org: Please review changes in content/common/BUILD.gn and content/common/sandbox_linux/bpf_gpu_policy_linux.cc Thank you
https://codereview.chromium.org/490233002/diff/180001/ui/ozone/public/surface... File ui/ozone/public/surface_factory_ozone.h (right): https://codereview.chromium.org/490233002/diff/180001/ui/ozone/public/surface... ui/ozone/public/surface_factory_ozone.h:88: virtual int GetDrmFd(); dnicoara reminded me to point out: We don't intend to approve hooks like this in general, as this function really is too prescriptive to be in the (platform & architecture-independent) ozone public interface. posciak@: We need to work to push VDA construction downwards (out of content), so that we can avoid adding more of these hooks.
My apologies for the delay in review, I'm not able to take a proper look at the moment. I will review in the next few days. Thanks for your understanding.
On 2014/09/11 05:11:25, Pawel Osciak wrote: > My apologies for the delay in review, I'm not able to take a proper look at the > moment. > I will review in the next few days. Thanks for your understanding. Hi Pawel, Thanks for the update. I managed to fix the remaining problem with the first video decode test (the one that doesn't render anything) causing Mesa to exit(), with this patch for libdrm : http://lists.freedesktop.org/archives/dri-devel/2014-September/068057.html Hopefully that gets upstream quick and I will submit it to Chromium OS' repo too. Cheers.
jiajia.qin@intel.com changed reviewers: + jiajia.qin@intel.com
I am not the reviewer. Just have some doubts. https://codereview.chromium.org/490233002/diff/180001/content/common/gpu/medi... File content/common/gpu/media/vaapi_picture_provider.cc (right): https://codereview.chromium.org/490233002/diff/180001/content/common/gpu/medi... content/common/gpu/media/vaapi_picture_provider.cc:442: VAProcPipelineParameterBuffer* pipeline_param; Using VPP seems like a little complicated. Do vaGetImage and eglCreateImageKHR achieve the same effect? https://codereview.chromium.org/490233002/diff/180001/content/common/gpu/medi... File content/common/gpu/media/vaapi_wrapper.cc (right): https://codereview.chromium.org/490233002/diff/180001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.cc:158: va_display = vaGetDisplayDRM(factory->GetDrmFd()); Hi, llandwerlin. Does it work for other external display, such as ozone-wayland? https://codereview.chromium.org/490233002/diff/180001/content/common/sandbox_... File content/common/sandbox_linux/bpf_gpu_policy_linux.cc (right): https://codereview.chromium.org/490233002/diff/180001/content/common/sandbox_... content/common/sandbox_linux/bpf_gpu_policy_linux.cc:236: dlopen("libva-drm.so.1", RTLD_NOW | RTLD_GLOBAL | RTLD_NODELETE); Just curious: why here still need to add this line since you have done it in VaapiWrapper::PostSandboxInitialization.
https://codereview.chromium.org/490233002/diff/180001/content/common/gpu/medi... File content/common/gpu/media/vaapi_picture_provider.cc (right): https://codereview.chromium.org/490233002/diff/180001/content/common/gpu/medi... content/common/gpu/media/vaapi_picture_provider.cc:442: VAProcPipelineParameterBuffer* pipeline_param; On 2014/09/16 11:09:55, jiajia.qin wrote: > Using VPP seems like a little complicated. Do vaGetImage and eglCreateImageKHR > achieve the same effect? On SandyBrige/IvyBridge this triggers exactly the same operation internally, except that with vaGetImage it doesn't seem possible to specify the color conversion matrix (but that won't make any difference because VPP shaders on IvyBridge have hardcoded BT709 matrix). https://codereview.chromium.org/490233002/diff/180001/content/common/gpu/medi... File content/common/gpu/media/vaapi_wrapper.cc (right): https://codereview.chromium.org/490233002/diff/180001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.cc:158: va_display = vaGetDisplayDRM(factory->GetDrmFd()); On 2014/09/16 11:09:55, jiajia.qin wrote: > Hi, llandwerlin. Does it work for other external display, such as ozone-wayland? What do you mean by external display? Are you using 2 different GPUs? https://codereview.chromium.org/490233002/diff/180001/content/common/sandbox_... File content/common/sandbox_linux/bpf_gpu_policy_linux.cc (right): https://codereview.chromium.org/490233002/diff/180001/content/common/sandbox_... content/common/sandbox_linux/bpf_gpu_policy_linux.cc:236: dlopen("libva-drm.so.1", RTLD_NOW | RTLD_GLOBAL | RTLD_NODELETE); On 2014/09/16 11:09:55, jiajia.qin wrote: > Just curious: why here still need to add this line since you have done it in > VaapiWrapper::PostSandboxInitialization. For the same reason there is the libva-x11.so.1 there. My understanding is that this file opens file descriptors to libraries before the sandbox actually happens. The InitializeStubs() call in vaapi_wrapper happens post sandbox, and just sets up the stubs pointers on already open libraries. Anything that hasn't been loaded through this file would fail.
https://codereview.chromium.org/490233002/diff/180001/content/common/gpu/medi... File content/common/gpu/media/vaapi_wrapper.cc (right): https://codereview.chromium.org/490233002/diff/180001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.cc:158: va_display = vaGetDisplayDRM(factory->GetDrmFd()); On 2014/09/16 11:09:55, jiajia.qin wrote: > Hi, llandwerlin. Does it work for other external display, such as ozone-wayland? No, it doesn't work with wayland. I'm hoping we can abstract it better soon, however for now ozone-wayland should be able to keep working via an alternate path (MediaOzonePlatform).
https://codereview.chromium.org/490233002/diff/180001/content/common/gpu/medi... File content/common/gpu/media/gpu_video_decode_accelerator.cc (right): https://codereview.chromium.org/490233002/diff/180001/content/common/gpu/medi... content/common/gpu/media/gpu_video_decode_accelerator.cc:275: #elif defined(OS_CHROMEOS) && defined(ARCH_CPU_X86_FAMILY) It seems that it conflicts with MediaOzonePlatform. For example, when use_ozone=1, it will never use MediaOzonePlatform since it will meet the condition defined(OS_CHROMEOS) && defined(ARCH_CPU_X86_FAMILY). This CL's target is making Vaapi accelerator work with ozone. So maybe you can further abstract it to work with wayland. In that case, We can remove MediaOzonePlatform directly. https://codereview.chromium.org/490233002/diff/180001/content/common/gpu/medi... File content/common/gpu/media/vaapi_picture_provider.cc (right): https://codereview.chromium.org/490233002/diff/180001/content/common/gpu/medi... content/common/gpu/media/vaapi_picture_provider.cc:346: factory->CreateNativePixmap(size, ui::SurfaceFactoryOzone::RGBA_8888); Wayland seems not support nativepixmap. If you use vaImage, it will avoid to use pixmap. That can let this provider becomes common for ozone-*, such as ozone-gbm, ozone-wayland. https://codereview.chromium.org/490233002/diff/180001/content/common/gpu/medi... content/common/gpu/media/vaapi_picture_provider.cc:442: VAProcPipelineParameterBuffer* pipeline_param; On 2014/09/16 12:28:58, llandwerlin wrote: > On 2014/09/16 11:09:55, jiajia.qin wrote: > > Using VPP seems like a little complicated. Do vaGetImage and eglCreateImageKHR > > achieve the same effect? > > On SandyBrige/IvyBridge this triggers exactly the same operation internally, > except that with vaGetImage it doesn't seem possible to specify the color > conversion matrix (but that won't make any difference because VPP shaders on > IvyBridge have hardcoded BT709 matrix). I add this comment because that I notice DrmVaapiPictureProvider::CreatePicture's body is not simple (same with this function). You need to config VASurfaceAttrib and VASurfaceAttribExternalBuffers. In addition, you used pixmap which may not be supported in wayland. Is there any simplified space? If you use vaGetImage, at lease, the code will be less that the current one.
https://codereview.chromium.org/490233002/diff/180001/content/common/gpu/medi... File content/common/gpu/media/gpu_video_decode_accelerator.cc (right): https://codereview.chromium.org/490233002/diff/180001/content/common/gpu/medi... content/common/gpu/media/gpu_video_decode_accelerator.cc:275: #elif defined(OS_CHROMEOS) && defined(ARCH_CPU_X86_FAMILY) On 2014/09/17 10:45:24, jiajia.qin wrote: > It seems that it conflicts with MediaOzonePlatform. For example, when > use_ozone=1, it will never use MediaOzonePlatform since it will meet the > condition defined(OS_CHROMEOS) && defined(ARCH_CPU_X86_FAMILY). This CL's target > is making Vaapi accelerator work with ozone. So maybe you can further abstract > it to work with wayland. In that case, We can remove MediaOzonePlatform > directly. Wayland builds for desktop (chromeos=0) and should hit the correct path.
Just adding the libdrm changes that are required for this change : https://chromium-review.googlesource.com/#/c/219251/ All this patches are upstream, it might be worth upgrading libdrm at the next release.
https://chromiumcodereview.appspot.com/490233002/diff/180001/content/common/g... File content/common/gpu/media/gpu_video_encode_accelerator.cc (right): https://chromiumcodereview.appspot.com/490233002/diff/180001/content/common/g... content/common/gpu/media/gpu_video_encode_accelerator.cc:193: const CommandLine* cmd_line = CommandLine::ForCurrentProcess(); Please don't remove base::. https://chromiumcodereview.appspot.com/490233002/diff/180001/content/common/g... File content/common/gpu/media/rendering_helper.h (right): https://chromiumcodereview.appspot.com/490233002/diff/180001/content/common/g... content/common/gpu/media/rendering_helper.h:126: class StubOzoneDelegate; #if defined(USE_OZONE) ? Or even move it down to where platform_window_delegate_ is defined? https://chromiumcodereview.appspot.com/490233002/diff/180001/content/common/g... File content/common/gpu/media/va_stub_header.fragment (right): https://chromiumcodereview.appspot.com/490233002/diff/180001/content/common/g... content/common/gpu/media/va_stub_header.fragment:9: #include "third_party/libva/va/va_drmcommon.h" Lexicographical order? https://chromiumcodereview.appspot.com/490233002/diff/180001/content/common/g... File content/common/gpu/media/vaapi_picture_provider.cc (right): https://chromiumcodereview.appspot.com/490233002/diff/180001/content/common/g... content/common/gpu/media/vaapi_picture_provider.cc:27: VAStatus va_status = input; \ I don't think we need this intermediate. https://chromiumcodereview.appspot.com/490233002/diff/180001/content/common/g... content/common/gpu/media/vaapi_picture_provider.cc:29: DVLOG(1) << err_msg << " : " << va_status; \ Could we use vaErrorStr()? https://chromiumcodereview.appspot.com/490233002/diff/180001/content/common/g... content/common/gpu/media/vaapi_picture_provider.cc:30: output_code; \ Please don't do it like this. Please move and destruction required to the destructor. https://chromiumcodereview.appspot.com/490233002/diff/180001/content/common/g... content/common/gpu/media/vaapi_picture_provider.cc:37: namespace { Do we need this namespace? https://chromiumcodereview.appspot.com/490233002/diff/180001/content/common/g... content/common/gpu/media/vaapi_picture_provider.cc:43: class X11VaapiPictureProvider : public VaapiPictureProvider { Could we have X11* and Drm* classes in separate files please? https://chromiumcodereview.appspot.com/490233002/diff/180001/content/common/g... content/common/gpu/media/vaapi_picture_provider.cc:65: Please no whitespace after public, private, etc. https://chromiumcodereview.appspot.com/490233002/diff/180001/content/common/g... content/common/gpu/media/vaapi_picture_provider.cc:89: virtual ~TFPPicture() { provider_->DestroyPicture(this); } Could we have this class destroy itself (passing x_display and mcc() instead of provider) instead? https://chromiumcodereview.appspot.com/490233002/diff/180001/content/common/g... content/common/gpu/media/vaapi_picture_provider.cc:127: XWindowAttributes win_attr; Please move this code back to TFPPicture::Initialize(). https://chromiumcodereview.appspot.com/490233002/diff/180001/content/common/g... content/common/gpu/media/vaapi_picture_provider.cc:180: bool X11VaapiPictureProvider::PutSurfaceIntoPicture( Please make it a virtual method in Picture and not cast to child. https://chromiumcodereview.appspot.com/490233002/diff/180001/content/common/g... content/common/gpu/media/vaapi_picture_provider.cc:235: bool X11VaapiPictureProvider::BindPicture(TFPPicture* tfp_picture) { Also could be moved back to TFPPicture. The provider should be doing what VAVDA used to, i.e. Initialize() the fb configs, but not more. Rest is TFPPicture-specific. https://chromiumcodereview.appspot.com/490233002/diff/180001/content/common/g... content/common/gpu/media/vaapi_picture_provider.cc:276: bool InitializeVpp(const gfx::Size& size); Please document this and other classes. https://chromiumcodereview.appspot.com/490233002/diff/180001/content/common/g... content/common/gpu/media/vaapi_picture_provider.cc:278: bool IsVppInitialized(); I don't think you will need this if you only DeinitializeVpp() in destructor and on size change? https://chromiumcodereview.appspot.com/490233002/diff/180001/content/common/g... content/common/gpu/media/vaapi_picture_provider.cc:383: if (status == VA_STATUS_SUCCESS) { Do we leak va_surface if not SUCCESS? https://chromiumcodereview.appspot.com/490233002/diff/180001/content/common/g... content/common/gpu/media/vaapi_picture_provider.cc:389: gl_image->Initialize( This can fail. https://chromiumcodereview.appspot.com/490233002/diff/180001/content/common/g... content/common/gpu/media/vaapi_picture_provider.cc:413: if (!make_context_current_.Run()) We still want to vaDestroySurfaces if this fails. https://chromiumcodereview.appspot.com/490233002/diff/180001/content/common/g... content/common/gpu/media/vaapi_picture_provider.cc:435: vaDestroySurfaces(va_display_, &va_surface, 1); Do we need to check if va_surface is valid? https://chromiumcodereview.appspot.com/490233002/diff/180001/content/common/g... content/common/gpu/media/vaapi_picture_provider.cc:447: LOG_VA_ERROR_AND_RETURN(vaMapBuffer(va_display_, Can we use own instance of VaapiWrapper for all va operations here? https://chromiumcodereview.appspot.com/490233002/diff/180001/content/common/g... content/common/gpu/media/vaapi_picture_provider.cc:450: "Couldn't map buffer",); Do we need to unmap on error? https://chromiumcodereview.appspot.com/490233002/diff/180001/content/common/g... content/common/gpu/media/vaapi_picture_provider.cc:468: LOG_VA_ERROR_AND_RETURN(vaUnmapBuffer(va_display_, vpp_buffer_), Would it make sense to keep the buffer mapped? Or do we need to unmap and map it every time? https://chromiumcodereview.appspot.com/490233002/diff/180001/content/common/g... content/common/gpu/media/vaapi_picture_provider.cc:485: return VA_STATUS_ERROR_OPERATION_FAILED; This method returns bool type. https://chromiumcodereview.appspot.com/490233002/diff/180001/content/common/g... content/common/gpu/media/vaapi_picture_provider.cc:489: drm_picture->gl_image()->BindTexImage(GL_TEXTURE_2D); This can fail. https://chromiumcodereview.appspot.com/490233002/diff/180001/content/common/g... content/common/gpu/media/vaapi_picture_provider.cc:491: if (static_cast<int>(glGetError()) != GL_NO_ERROR) Is this cast needed? https://chromiumcodereview.appspot.com/490233002/diff/180001/content/common/g... content/common/gpu/media/vaapi_picture_provider.cc:525: DeinitializeVpp()); I don't think you need this if you DeinitializeVpp in destructor? https://chromiumcodereview.appspot.com/490233002/diff/180001/content/common/g... content/common/gpu/media/vaapi_picture_provider.cc:570: scoped_ptr<VaapiPictureProvider> backend; s/backend/provider/ https://chromiumcodereview.appspot.com/490233002/diff/180001/content/common/g... content/common/gpu/media/vaapi_picture_provider.cc:585: return backend.Pass(); Pass() shouldn't be required. https://chromiumcodereview.appspot.com/490233002/diff/180001/content/common/g... File content/common/gpu/media/vaapi_picture_provider.h (right): https://chromiumcodereview.appspot.com/490233002/diff/180001/content/common/g... content/common/gpu/media/vaapi_picture_provider.h:20: }; // namespace gfx s/;// https://chromiumcodereview.appspot.com/490233002/diff/180001/content/common/g... content/common/gpu/media/vaapi_picture_provider.h:25: // window system and bind them to gl textures. s/bind/binding/ https://chromiumcodereview.appspot.com/490233002/diff/180001/content/common/g... content/common/gpu/media/vaapi_picture_provider.h:26: class VaapiPictureProvider { Please keep NonThreadSafe property on this and picture classes. https://chromiumcodereview.appspot.com/490233002/diff/180001/content/common/g... content/common/gpu/media/vaapi_picture_provider.h:55: static scoped_ptr<VaapiPictureProvider> Create( Please add documentation for methods. https://chromiumcodereview.appspot.com/490233002/diff/180001/content/common/g... content/common/gpu/media/vaapi_picture_provider.h:67: virtual bool SetCodedSurfacesSize(const gfx::Size& size) { return true; } I don't understand this default body... When does this make sense? https://chromiumcodereview.appspot.com/490233002/diff/180001/content/common/g... content/common/gpu/media/vaapi_picture_provider.h:74: #endif // CONTENT_COMMON_GPU_MEDIA_VAAPI_PICTURE_PROVIDER_H_ Two spaces before '//'. https://chromiumcodereview.appspot.com/490233002/diff/180001/content/common/g... File content/common/gpu/media/vaapi_video_decode_accelerator.cc (right): https://chromiumcodereview.appspot.com/490233002/diff/180001/content/common/g... content/common/gpu/media/vaapi_video_decode_accelerator.cc:92: pictures_.clear(); Looks like each Picture should rather have a refptr to the provider. https://chromiumcodereview.appspot.com/490233002/diff/180001/content/common/g... content/common/gpu/media/vaapi_video_decode_accelerator.cc:131: vaapi_wrapper_->GetDisplay(), Could the provider get its own display? Or better, as mentioned before, use its own VaapiWrapper? https://chromiumcodereview.appspot.com/490233002/diff/180001/content/common/g... content/common/gpu/media/vaapi_video_decode_accelerator.cc:545: DCHECK(!pictures_.contains(buffers[i].id())); I think this should be s/!// ? Are you testing on a Debug build? https://chromiumcodereview.appspot.com/490233002/diff/180001/content/common/g... File content/common/gpu/media/vaapi_video_decode_accelerator.h (right): https://chromiumcodereview.appspot.com/490233002/diff/180001/content/common/g... content/common/gpu/media/vaapi_video_decode_accelerator.h:198: typedef base::ScopedPtrHashMap<int32, VaapiPictureProvider::Picture> Pictures; Why do we need a hashmap? https://chromiumcodereview.appspot.com/490233002/diff/180001/content/common/g... content/common/gpu/media/vaapi_video_decode_accelerator.h:203: // Return a TFPPicture associated with given client-provided id. s/TFPPicture/Picture/ https://chromiumcodereview.appspot.com/490233002/diff/180001/content/common/g... File content/common/gpu/media/vaapi_video_encode_accelerator.h (right): https://chromiumcodereview.appspot.com/490233002/diff/180001/content/common/g... content/common/gpu/media/vaapi_video_encode_accelerator.h:22: }; // namespace gfx Unused. https://chromiumcodereview.appspot.com/490233002/diff/180001/content/common/g... content/common/gpu/media/vaapi_video_encode_accelerator.h:32: explicit VaapiVideoEncodeAccelerator(); No need for explicit anymore. https://chromiumcodereview.appspot.com/490233002/diff/180001/content/common/g... File content/common/gpu/media/vaapi_wrapper.h (right): https://chromiumcodereview.appspot.com/490233002/diff/180001/content/common/g... content/common/gpu/media/vaapi_wrapper.h:133: Please don't add newlines. https://chromiumcodereview.appspot.com/490233002/diff/180001/content/common/g... File content/common/gpu/media/video_decode_accelerator_unittest.cc (right): https://chromiumcodereview.appspot.com/490233002/diff/180001/content/common/g... content/common/gpu/media/video_decode_accelerator_unittest.cc:384: #if defined(OS_CHROMEOS) && defined(ARCH_CPU_X86_FAMILY) Why were we ok without this before? https://chromiumcodereview.appspot.com/490233002/diff/180001/content/common/g... content/common/gpu/media/video_decode_accelerator_unittest.cc:385: static bool MakeDecoderCurrent(scoped_refptr<gfx::GLContext> context) { s/MakeDecoderCurrent/MakeContextCurrent/ https://chromiumcodereview.appspot.com/490233002/diff/180001/media/test/data/... File media/test/data/test-25fps.h264.md5 (right): https://chromiumcodereview.appspot.com/490233002/diff/180001/media/test/data/... media/test/data/test-25fps.h264.md5:3: # Intel Ivy Bridge - EGL Could we update other platforms as well please?
https://codereview.chromium.org/490233002/diff/180001/content/common/gpu/medi... File content/common/gpu/media/vaapi_picture_provider.cc (right): https://codereview.chromium.org/490233002/diff/180001/content/common/gpu/medi... content/common/gpu/media/vaapi_picture_provider.cc:27: VAStatus va_status = input; \ On 2014/09/24 11:27:11, Pawel Osciak wrote: > I don't think we need this intermediate. I will need it if I'm using vaErrorStr(). https://codereview.chromium.org/490233002/diff/180001/content/common/gpu/medi... content/common/gpu/media/vaapi_picture_provider.cc:29: DVLOG(1) << err_msg << " : " << va_status; \ On 2014/09/24 11:27:11, Pawel Osciak wrote: > Could we use vaErrorStr()? Acknowledged. https://codereview.chromium.org/490233002/diff/180001/content/common/gpu/medi... content/common/gpu/media/vaapi_picture_provider.cc:30: output_code; \ On 2014/09/24 11:27:11, Pawel Osciak wrote: > Please don't do it like this. Please move and destruction required to the > destructor. Acknowledged. https://codereview.chromium.org/490233002/diff/180001/content/common/gpu/medi... content/common/gpu/media/vaapi_picture_provider.cc:37: namespace { On 2014/09/24 11:27:12, Pawel Osciak wrote: > Do we need this namespace? I'm just trying to remove stuff from the content namespace since it's implementation details of the VaapiPictureProvider. I will remove. https://codereview.chromium.org/490233002/diff/180001/content/common/gpu/medi... content/common/gpu/media/vaapi_picture_provider.cc:37: namespace { On 2014/09/24 11:27:12, Pawel Osciak wrote: > Do we need this namespace? Acknowledged. https://codereview.chromium.org/490233002/diff/180001/content/common/gpu/medi... content/common/gpu/media/vaapi_picture_provider.cc:43: class X11VaapiPictureProvider : public VaapiPictureProvider { On 2014/09/24 11:27:10, Pawel Osciak wrote: > Could we have X11* and Drm* classes in separate files please? Acknowledged. https://codereview.chromium.org/490233002/diff/180001/content/common/gpu/medi... content/common/gpu/media/vaapi_picture_provider.cc:65: On 2014/09/24 11:27:11, Pawel Osciak wrote: > Please no whitespace after public, private, etc. Acknowledged. https://codereview.chromium.org/490233002/diff/180001/content/common/gpu/medi... content/common/gpu/media/vaapi_picture_provider.cc:180: bool X11VaapiPictureProvider::PutSurfaceIntoPicture( On 2014/09/24 11:27:11, Pawel Osciak wrote: > Please make it a virtual method in Picture and not cast to child. Acknowledged. https://codereview.chromium.org/490233002/diff/180001/content/common/gpu/medi... content/common/gpu/media/vaapi_picture_provider.cc:235: bool X11VaapiPictureProvider::BindPicture(TFPPicture* tfp_picture) { On 2014/09/24 11:27:10, Pawel Osciak wrote: > Also could be moved back to TFPPicture. > > The provider should be doing what VAVDA used to, i.e. Initialize() the fb > configs, but not more. Rest is TFPPicture-specific. Acknowledged. https://codereview.chromium.org/490233002/diff/180001/content/common/gpu/medi... content/common/gpu/media/vaapi_picture_provider.cc:276: bool InitializeVpp(const gfx::Size& size); On 2014/09/24 11:27:11, Pawel Osciak wrote: > Please document this and other classes. Acknowledged. https://codereview.chromium.org/490233002/diff/180001/content/common/gpu/medi... content/common/gpu/media/vaapi_picture_provider.cc:278: bool IsVppInitialized(); On 2014/09/24 11:27:11, Pawel Osciak wrote: > I don't think you will need this if you only DeinitializeVpp() in destructor and > on size change? Acknowledged. https://codereview.chromium.org/490233002/diff/180001/content/common/gpu/medi... content/common/gpu/media/vaapi_picture_provider.cc:383: if (status == VA_STATUS_SUCCESS) { On 2014/09/24 11:27:11, Pawel Osciak wrote: > Do we leak va_surface if not SUCCESS? Thanks, moving this all back into TFPPicture so it's properly released when the picture is deleted. https://codereview.chromium.org/490233002/diff/180001/content/common/gpu/medi... content/common/gpu/media/vaapi_picture_provider.cc:389: gl_image->Initialize( On 2014/09/24 11:27:10, Pawel Osciak wrote: > This can fail. Right, I was counting on the following glGetError(), but will check the return value inside the Initialize() of TFPPicture instead. https://codereview.chromium.org/490233002/diff/180001/content/common/gpu/medi... content/common/gpu/media/vaapi_picture_provider.cc:413: if (!make_context_current_.Run()) On 2014/09/24 11:27:11, Pawel Osciak wrote: > We still want to vaDestroySurfaces if this fails. Acknowledged. https://codereview.chromium.org/490233002/diff/180001/content/common/gpu/medi... content/common/gpu/media/vaapi_picture_provider.cc:435: vaDestroySurfaces(va_display_, &va_surface, 1); On 2014/09/24 11:27:10, Pawel Osciak wrote: > Do we need to check if va_surface is valid? Acknowledged. https://codereview.chromium.org/490233002/diff/180001/content/common/gpu/medi... content/common/gpu/media/vaapi_picture_provider.cc:447: LOG_VA_ERROR_AND_RETURN(vaMapBuffer(va_display_, On 2014/09/24 11:27:12, Pawel Osciak wrote: > Can we use own instance of VaapiWrapper for all va operations here? Would you mind if I did this in another CL? It feels like it's going to make VaapiWrapper more complicated. https://codereview.chromium.org/490233002/diff/180001/content/common/gpu/medi... content/common/gpu/media/vaapi_picture_provider.cc:450: "Couldn't map buffer",); On 2014/09/24 11:27:10, Pawel Osciak wrote: > Do we need to unmap on error? Either MapBuffer works or it fails and nothing gets mapped. Not sure what could be the problem here, given that no call could fail between a successful map and an unmap. https://codereview.chromium.org/490233002/diff/180001/content/common/gpu/medi... content/common/gpu/media/vaapi_picture_provider.cc:468: LOG_VA_ERROR_AND_RETURN(vaUnmapBuffer(va_display_, vpp_buffer_), On 2014/09/24 11:27:11, Pawel Osciak wrote: > Would it make sense to keep the buffer mapped? Or do we need to unmap and map it > every time? On Intel hardware we need to unmap the buffer object (ie flush the cpu cache) so the hardware gets the right data. https://codereview.chromium.org/490233002/diff/180001/content/common/gpu/medi... content/common/gpu/media/vaapi_picture_provider.cc:485: return VA_STATUS_ERROR_OPERATION_FAILED; On 2014/09/24 11:27:12, Pawel Osciak wrote: > This method returns bool type. Acknowledged. https://codereview.chromium.org/490233002/diff/180001/content/common/gpu/medi... content/common/gpu/media/vaapi_picture_provider.cc:489: drm_picture->gl_image()->BindTexImage(GL_TEXTURE_2D); On 2014/09/24 11:27:12, Pawel Osciak wrote: > This can fail. Acknowledged. https://codereview.chromium.org/490233002/diff/180001/content/common/gpu/medi... content/common/gpu/media/vaapi_picture_provider.cc:491: if (static_cast<int>(glGetError()) != GL_NO_ERROR) On 2014/09/24 11:27:11, Pawel Osciak wrote: > Is this cast needed? Acknowledged. https://codereview.chromium.org/490233002/diff/180001/content/common/gpu/medi... content/common/gpu/media/vaapi_picture_provider.cc:525: DeinitializeVpp()); On 2014/09/24 11:27:11, Pawel Osciak wrote: > I don't think you need this if you DeinitializeVpp in destructor? Acknowledged. https://codereview.chromium.org/490233002/diff/180001/content/common/gpu/medi... content/common/gpu/media/vaapi_picture_provider.cc:570: scoped_ptr<VaapiPictureProvider> backend; On 2014/09/24 11:27:11, Pawel Osciak wrote: > s/backend/provider/ Acknowledged. https://codereview.chromium.org/490233002/diff/180001/content/common/gpu/medi... content/common/gpu/media/vaapi_picture_provider.cc:585: return backend.Pass(); On 2014/09/24 11:27:11, Pawel Osciak wrote: > Pass() shouldn't be required. Sadly it doesn't compile without it : src/base/memory/scoped_ptr.h: In static member function 'static scoped_ptr<content::VaapiPictureProvider> content::VaapiPictureProvider::Create(VADisplay, gfx::GLContext*, base::Callback<bool()>)': src/base/memory/scoped_ptr.h:311:107: error: 'scoped_ptr<T, D>::scoped_ptr(scoped_ptr<T, D>&) [with T = content::VaapiPictureProvider; D = base::DefaultDeleter<content::VaapiPictureProvider>]' is private MOVE_ONLY_TYPE_FOR_CPP_03(scoped_ptr, RValue) https://codereview.chromium.org/490233002/diff/180001/content/common/gpu/medi... File content/common/gpu/media/vaapi_picture_provider.h (right): https://codereview.chromium.org/490233002/diff/180001/content/common/gpu/medi... content/common/gpu/media/vaapi_picture_provider.h:20: }; // namespace gfx On 2014/09/24 11:27:12, Pawel Osciak wrote: > s/;// Acknowledged. https://codereview.chromium.org/490233002/diff/180001/content/common/gpu/medi... content/common/gpu/media/vaapi_picture_provider.h:25: // window system and bind them to gl textures. On 2014/09/24 11:27:12, Pawel Osciak wrote: > s/bind/binding/ Acknowledged. https://codereview.chromium.org/490233002/diff/180001/content/common/gpu/medi... content/common/gpu/media/vaapi_picture_provider.h:26: class VaapiPictureProvider { On 2014/09/24 11:27:12, Pawel Osciak wrote: > Please keep NonThreadSafe property on this and picture classes. Acknowledged. https://codereview.chromium.org/490233002/diff/180001/content/common/gpu/medi... content/common/gpu/media/vaapi_picture_provider.h:55: static scoped_ptr<VaapiPictureProvider> Create( On 2014/09/24 11:27:12, Pawel Osciak wrote: > Please add documentation for methods. Acknowledged. https://codereview.chromium.org/490233002/diff/180001/content/common/gpu/medi... content/common/gpu/media/vaapi_picture_provider.h:67: virtual bool SetCodedSurfacesSize(const gfx::Size& size) { return true; } On 2014/09/24 11:27:12, Pawel Osciak wrote: > I don't understand this default body... When does this make sense? Making it virtual and moving this body to the X11 implementation. https://codereview.chromium.org/490233002/diff/180001/content/common/gpu/medi... content/common/gpu/media/vaapi_picture_provider.h:74: #endif // CONTENT_COMMON_GPU_MEDIA_VAAPI_PICTURE_PROVIDER_H_ On 2014/09/24 11:27:12, Pawel Osciak wrote: > Two spaces before '//'. Acknowledged. https://codereview.chromium.org/490233002/diff/180001/media/test/data/test-25... File media/test/data/test-25fps.h264.md5 (right): https://codereview.chromium.org/490233002/diff/180001/media/test/data/test-25... media/test/data/test-25fps.h264.md5:3: # Intel Ivy Bridge - EGL On 2014/09/24 11:27:13, Pawel Osciak wrote: > Could we update other platforms as well please? I'm going to give a go at the Haswell, but I couldn't get my SandyBrige system to boot a freon image. That's a LeNovo X220, what SandyBridge system are you using?
PTAL, Thanks.
On 2014/10/01 12:18:03, llandwerlin wrote: > PTAL, Thanks. There are still a few remaining unaddressed comments from PS10 in accelerator classes. Could you please respond to them? Thanks.
Thanks a lot for the missing items. Here are the responses for the unanswered questions. https://codereview.chromium.org/490233002/diff/180001/content/common/gpu/medi... File content/common/gpu/media/vaapi_video_decode_accelerator.cc (right): https://codereview.chromium.org/490233002/diff/180001/content/common/gpu/medi... content/common/gpu/media/vaapi_video_decode_accelerator.cc:92: pictures_.clear(); On 2014/09/24 11:27:12, Pawel Osciak wrote: > Looks like each Picture should rather have a refptr to the provider. Right, thanks, the comment is misleading here (leftover from previous version of the CL). But with the changes you asked for (every picture should be able to destroy itself) no pointer to the VaapiPictureProvider is required. I'll add a comment to mention that pictures need to be destroyed before the VaapiWrapper otherwise their VADisplay might be invalid. https://codereview.chromium.org/490233002/diff/180001/content/common/gpu/medi... content/common/gpu/media/vaapi_video_decode_accelerator.cc:131: vaapi_wrapper_->GetDisplay(), On 2014/09/24 11:27:12, Pawel Osciak wrote: > Could the provider get its own display? Or better, as mentioned before, use its > own VaapiWrapper? You need to have the all VA objects (VASurface/VAContext/etc...) allocated from the same display object (parent object) in order to do operations across them (VPP stuff or call vaPutSurface()). If it's not the case, you will experience crashes in libva. Would you like me to give the VaapiWrapper object instead? https://codereview.chromium.org/490233002/diff/180001/content/common/gpu/medi... content/common/gpu/media/vaapi_video_decode_accelerator.cc:545: DCHECK(!pictures_.contains(buffers[i].id())); On 2014/09/24 11:27:12, Pawel Osciak wrote: > I think this should be s/!// ? > > Are you testing on a Debug build? I might be wrong, but the purpose here is to check that the caller didn't provide 2 buffers with the same id. Therefore it should be "not contained". I'm using the version 9999 of chromeos-chrome package, not sure if that enables dchecks. https://codereview.chromium.org/490233002/diff/180001/content/common/gpu/medi... File content/common/gpu/media/vaapi_video_decode_accelerator.h (right): https://codereview.chromium.org/490233002/diff/180001/content/common/gpu/medi... content/common/gpu/media/vaapi_video_decode_accelerator.h:198: typedef base::ScopedPtrHashMap<int32, VaapiPictureProvider::Picture> Pictures; On 2014/09/24 11:27:12, Pawel Osciak wrote: > Why do we need a hashmap? I couldn't find a data structure with the same properties as std::map that would also call delete on the pointer stored. Would std::map<int32, scoped_ptr<VaapiPictureProvider::Picture> > be ok? https://codereview.chromium.org/490233002/diff/180001/content/common/gpu/medi... content/common/gpu/media/vaapi_video_decode_accelerator.h:203: // Return a TFPPicture associated with given client-provided id. On 2014/09/24 11:27:12, Pawel Osciak wrote: > s/TFPPicture/Picture/ Acknowledged. https://codereview.chromium.org/490233002/diff/180001/content/common/gpu/medi... File content/common/gpu/media/vaapi_video_encode_accelerator.h (right): https://codereview.chromium.org/490233002/diff/180001/content/common/gpu/medi... content/common/gpu/media/vaapi_video_encode_accelerator.h:22: }; // namespace gfx On 2014/09/24 11:27:12, Pawel Osciak wrote: > Unused. Acknowledged. https://codereview.chromium.org/490233002/diff/180001/content/common/gpu/medi... content/common/gpu/media/vaapi_video_encode_accelerator.h:32: explicit VaapiVideoEncodeAccelerator(); On 2014/09/24 11:27:12, Pawel Osciak wrote: > No need for explicit anymore. Acknowledged.
https://codereview.chromium.org/490233002/diff/180001/content/common/gpu/medi... File content/common/gpu/media/vaapi_picture_provider.cc (right): https://codereview.chromium.org/490233002/diff/180001/content/common/gpu/medi... content/common/gpu/media/vaapi_picture_provider.cc:447: LOG_VA_ERROR_AND_RETURN(vaMapBuffer(va_display_, On 2014/09/25 09:54:18, llandwerlin wrote: > On 2014/09/24 11:27:12, Pawel Osciak wrote: > > Can we use own instance of VaapiWrapper for all va operations here? > > Would you mind if I did this in another CL? > It feels like it's going to make VaapiWrapper more complicated. I'm open to the suggestion, but perhaps we could consider doing this please? It feels to me that it could actually give us less code here, I think all the va functions you are using here are already implemented in the wrapper. Is there more to it than just calling vaapi wrapper methods here instead of va functions directly? And we could get rid of resource management here and let VW do it for us... And we have locking in the wrapper which you are not doing here... https://codereview.chromium.org/490233002/diff/180001/content/common/gpu/medi... content/common/gpu/media/vaapi_picture_provider.cc:450: "Couldn't map buffer",); On 2014/09/25 09:54:17, llandwerlin wrote: > On 2014/09/24 11:27:10, Pawel Osciak wrote: > > Do we need to unmap on error? > > Either MapBuffer works or it fails and nothing gets mapped. > Not sure what could be the problem here, given that no call could fail between a > successful map and an unmap. Acknowledged. https://codereview.chromium.org/490233002/diff/180001/content/common/gpu/medi... content/common/gpu/media/vaapi_picture_provider.cc:468: LOG_VA_ERROR_AND_RETURN(vaUnmapBuffer(va_display_, vpp_buffer_), On 2014/09/25 09:54:18, llandwerlin wrote: > On 2014/09/24 11:27:11, Pawel Osciak wrote: > > Would it make sense to keep the buffer mapped? Or do we need to unmap and map > it > > every time? > > On Intel hardware we need to unmap the buffer object (ie flush the cpu cache) so > the hardware gets the right data. Acknowledged. https://codereview.chromium.org/490233002/diff/180001/content/common/gpu/medi... content/common/gpu/media/vaapi_picture_provider.cc:585: return backend.Pass(); On 2014/09/25 09:54:17, llandwerlin wrote: > On 2014/09/24 11:27:11, Pawel Osciak wrote: > > Pass() shouldn't be required. > > Sadly it doesn't compile without it : > > src/base/memory/scoped_ptr.h: In static member function 'static > scoped_ptr<content::VaapiPictureProvider> > content::VaapiPictureProvider::Create(VADisplay, gfx::GLContext*, > base::Callback<bool()>)': > src/base/memory/scoped_ptr.h:311:107: error: 'scoped_ptr<T, > D>::scoped_ptr(scoped_ptr<T, D>&) [with T = content::VaapiPictureProvider; D = > base::DefaultDeleter<content::VaapiPictureProvider>]' is private > MOVE_ONLY_TYPE_FOR_CPP_03(scoped_ptr, RValue) Right, sorry, I misread scoped_ptr as refptr. https://codereview.chromium.org/490233002/diff/180001/content/common/gpu/medi... File content/common/gpu/media/vaapi_video_decode_accelerator.cc (right): https://codereview.chromium.org/490233002/diff/180001/content/common/gpu/medi... content/common/gpu/media/vaapi_video_decode_accelerator.cc:131: vaapi_wrapper_->GetDisplay(), On 2014/10/02 14:14:23, llandwerlin wrote: > On 2014/09/24 11:27:12, Pawel Osciak wrote: > > Could the provider get its own display? Or better, as mentioned before, use > its > > own VaapiWrapper? > > You need to have the all VA objects (VASurface/VAContext/etc...) allocated from > the same display object (parent object) in order to do operations across them > (VPP stuff or call vaPutSurface()). If it's not the case, you will experience > crashes in libva. > > Would you like me to give the VaapiWrapper object instead? Yes, please. I'd really prefer to use a common VaapiWrapper. Then we wouldn't have to worry about ownership, lifetimes, synchronization and same display...? https://codereview.chromium.org/490233002/diff/180001/content/common/gpu/medi... content/common/gpu/media/vaapi_video_decode_accelerator.cc:545: DCHECK(!pictures_.contains(buffers[i].id())); On 2014/10/02 14:14:23, llandwerlin wrote: > On 2014/09/24 11:27:12, Pawel Osciak wrote: > > I think this should be s/!// ? > > > > Are you testing on a Debug build? > > I might be wrong, but the purpose here is to check that the caller didn't > provide 2 buffers with the same id. Therefore it should be "not contained". > > I'm using the version 9999 of chromeos-chrome package, not sure if that enables > dchecks. Ah you are right, sorry. As for building chrome, I'd recommend http://www.chromium.org/chromium-os/how-tos-and-troubleshooting/building-chro.... https://codereview.chromium.org/490233002/diff/180001/content/common/gpu/medi... File content/common/gpu/media/vaapi_video_decode_accelerator.h (right): https://codereview.chromium.org/490233002/diff/180001/content/common/gpu/medi... content/common/gpu/media/vaapi_video_decode_accelerator.h:198: typedef base::ScopedPtrHashMap<int32, VaapiPictureProvider::Picture> Pictures; On 2014/10/02 14:14:23, llandwerlin wrote: > On 2014/09/24 11:27:12, Pawel Osciak wrote: > > Why do we need a hashmap? > > I couldn't find a data structure with the same properties as std::map that would > also call delete on the pointer stored. > Would std::map<int32, scoped_ptr<VaapiPictureProvider::Picture> > be ok? linked_ptr currently used does delete... https://codereview.chromium.org/490233002/diff/180001/content/common/gpu/medi... File content/common/gpu/media/video_decode_accelerator_unittest.cc (right): https://codereview.chromium.org/490233002/diff/180001/content/common/gpu/medi... content/common/gpu/media/video_decode_accelerator_unittest.cc:384: #if defined(OS_CHROMEOS) && defined(ARCH_CPU_X86_FAMILY) On 2014/09/24 11:27:13, Pawel Osciak wrote: > Why were we ok without this before? Question still stands please. https://codereview.chromium.org/490233002/diff/180001/content/common/gpu/medi... content/common/gpu/media/video_decode_accelerator_unittest.cc:385: static bool MakeDecoderCurrent(scoped_refptr<gfx::GLContext> context) { On 2014/09/24 11:27:13, Pawel Osciak wrote: > s/MakeDecoderCurrent/MakeContextCurrent/ I think MakeContextCurrent is still a better name? https://codereview.chromium.org/490233002/diff/220001/content/common/gpu/medi... File content/common/gpu/media/gpu_video_decode_accelerator.cc (right): https://codereview.chromium.org/490233002/diff/220001/content/common/gpu/medi... content/common/gpu/media/gpu_video_decode_accelerator.cc:278: #elif defined(USE_OZONE) What is happening here now that we go through the case above? Is this dead code? https://codereview.chromium.org/490233002/diff/220001/content/common/gpu/medi... File content/common/gpu/media/rendering_helper.cc (right): https://codereview.chromium.org/490233002/diff/220001/content/common/gpu/medi... content/common/gpu/media/rendering_helper.cc:87: ui::PlatformWindowState new_state) OVERRIDE{}; I think we need a space before OVERRIDE (I may be wrong)? Please make sure to run git cl format on the CL before uploading. https://codereview.chromium.org/490233002/diff/220001/content/common/gpu/medi... File content/common/gpu/media/vaapi_picture_provider.cc (right): https://codereview.chromium.org/490233002/diff/220001/content/common/gpu/medi... content/common/gpu/media/vaapi_picture_provider.cc:8: #if !defined(USE_X11) Conditionally compiled headers should go below the regular ones. https://codereview.chromium.org/490233002/diff/220001/content/common/gpu/medi... File content/common/gpu/media/vaapi_picture_provider.h (right): https://codereview.chromium.org/490233002/diff/220001/content/common/gpu/medi... content/common/gpu/media/vaapi_picture_provider.h:35: }; Spurious ; https://codereview.chromium.org/490233002/diff/220001/content/common/gpu/medi... content/common/gpu/media/vaapi_picture_provider.h:39: virtual bool PutSurface(VASurfaceID va_surface_id) = 0; "Put" may suggest releasing the surface/resources. Perhaps DownloadFromSurface? https://codereview.chromium.org/490233002/diff/220001/content/common/gpu/medi... content/common/gpu/media/vaapi_picture_provider.h:67: // Notify the picture provider of the picture |size| change. Please document return value and what happens, it's not obvious. https://codereview.chromium.org/490233002/diff/220001/content/common/gpu/medi... content/common/gpu/media/vaapi_picture_provider.h:75: }; Please DISALLOW_COPY_AND_ASSIGN here and in Picture classes. https://codereview.chromium.org/490233002/diff/220001/content/common/gpu/medi... File content/common/gpu/media/vaapi_picture_provider_drm.cc (right): https://codereview.chromium.org/490233002/diff/220001/content/common/gpu/medi... content/common/gpu/media/vaapi_picture_provider_drm.cc:60: CHECK_EQ(glGetError(), GL_NO_ERROR); I think this is not critical enough to crash the process? Perhaps DCHECK would be sufficient... https://codereview.chromium.org/490233002/diff/220001/content/common/gpu/medi... content/common/gpu/media/vaapi_picture_provider_drm.cc:75: factory->CreateNativePixmap(size(), ui::SurfaceFactoryOzone::RGBA_8888); Check for NULL? https://codereview.chromium.org/490233002/diff/220001/content/common/gpu/medi... content/common/gpu/media/vaapi_picture_provider_drm.cc:76: unsigned long buffer_fd = pixmap_->GetDmaBufFd(); fd is an int... We should also check for -1. https://codereview.chromium.org/490233002/diff/220001/content/common/gpu/medi... content/common/gpu/media/vaapi_picture_provider_drm.cc:160: DrmPicture* drm_picture = new DrmPicture(this, Could we create a scoped_ptr directly here? scoped_ptr<Picture> picture(new DrmPicture(...)). https://codereview.chromium.org/490233002/diff/220001/content/common/gpu/medi... content/common/gpu/media/vaapi_picture_provider_drm.cc:166: scoped_ptr<VaapiPictureProvider::Picture> picture(drm_picture); Do we need the qualifier here if we are child of VaapiPictureProvider? https://codereview.chromium.org/490233002/diff/220001/content/common/gpu/medi... content/common/gpu/media/vaapi_picture_provider_drm.cc:217: bool DrmVaapiPictureProvider::SetCodedSurfacesSize(const gfx::Size& size) { I realized, could we just do this on PutSurfaceIntoPicture? Check if size passed to it is different than previous size and if so, reinitialize? https://codereview.chromium.org/490233002/diff/220001/content/common/gpu/medi... File content/common/gpu/media/vaapi_picture_provider_drm.h (right): https://codereview.chromium.org/490233002/diff/220001/content/common/gpu/medi... content/common/gpu/media/vaapi_picture_provider_drm.h:35: bool PutSurfaceIntoPicture(VASurfaceID va_surface_id_src, This should be private? https://codereview.chromium.org/490233002/diff/220001/content/common/gpu/medi... File content/common/gpu/media/vaapi_picture_provider_x11.cc (right): https://codereview.chromium.org/490233002/diff/220001/content/common/gpu/medi... content/common/gpu/media/vaapi_picture_provider_x11.cc:94: LOG_VA_ERROR_AND_RETURN(vaPutSurface(va_display_, Could we please keep using the VaapiWrapper instance that VAVDA uses, passing the wrapper instead of va display into the provider? Especially since it's the owner of VASurface. https://codereview.chromium.org/490233002/diff/220001/content/common/gpu/medi... File content/common/gpu/media/vaapi_video_decode_accelerator.cc (right): https://codereview.chromium.org/490233002/diff/220001/content/common/gpu/medi... content/common/gpu/media/vaapi_video_decode_accelerator.cc:91: // Make sure pictures are destroyed before VaapiWrapper is, so the If we keep a refcount to the wrapper in provider, this should not be needed. https://codereview.chromium.org/490233002/diff/220001/content/common/gpu/medi... File content/common/gpu/media/vaapi_video_decode_accelerator.h (right): https://codereview.chromium.org/490233002/diff/220001/content/common/gpu/medi... content/common/gpu/media/vaapi_video_decode_accelerator.h:151: // Client-provided X/GLX state. Stale comment. https://codereview.chromium.org/490233002/diff/220001/content/common/gpu/medi... content/common/gpu/media/vaapi_video_decode_accelerator.h:246: scoped_ptr<VaapiPictureProvider> vaapi_picture_provider_; We should add a comment that it is important this outlives wrapper, and has to stay defined after it for proper destruction order. https://codereview.chromium.org/490233002/diff/220001/content/common/gpu/medi... File content/common/gpu/media/vaapi_wrapper.cc (right): https://codereview.chromium.org/490233002/diff/220001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.cc:158: va_display = vaGetDisplayDRM(factory->GetDrmFd()); Do we need to close the fd somewhere? https://codereview.chromium.org/490233002/diff/220001/content/common/gpu/medi... File content/common/gpu/media/vaapi_wrapper.h (right): https://codereview.chromium.org/490233002/diff/220001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.h:129: // Returns the initialized VADisplay I feel really uncomfortable about this. We should either reuse the wrapper instance, or wrap and refcount the display (but that would be an overkill, so I prefer the former). https://codereview.chromium.org/490233002/diff/220001/content/common/gpu/medi... File content/common/gpu/media/video_decode_accelerator_unittest.cc (right): https://codereview.chromium.org/490233002/diff/220001/content/common/gpu/medi... content/common/gpu/media/video_decode_accelerator_unittest.cc:385: static bool MakeDecoderCurrent(scoped_refptr<gfx::GLContext> context) { MakeContextCurrent https://codereview.chromium.org/490233002/diff/220001/content/common/gpu/medi... content/common/gpu/media/video_decode_accelerator_unittest.cc:877: #if defined(OS_WIN) || defined(USE_OZONE) Is this also required for ozone? The comment below seems to be very Windows-specific. https://codereview.chromium.org/490233002/diff/220001/content/common/gpu/medi... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/490233002/diff/220001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:1035: ui::OzonePlatform::InitializeForUI(); There is no rendering in this test. Do we need these? https://codereview.chromium.org/490233002/diff/220001/ui/ozone/platform/dri/g... File ui/ozone/platform/dri/gbm_buffer.cc (right): https://codereview.chromium.org/490233002/diff/220001/ui/ozone/platform/dri/g... ui/ozone/platform/dri/gbm_buffer.cc:74: return gbm_bo_get_fd(buffer_->bo()); I think we should be closing the fd somewhere once we are done with it?
Pawel, answering your questions quickly. I won't be able to upload a new revision before next week. I still do have a question about the VaapiWrapper, do you prefer it refcounted or should I fiddle around with the destruction order of members of VaapiVideoDecoderAccelerator? Thanks. https://codereview.chromium.org/490233002/diff/180001/content/common/gpu/medi... File content/common/gpu/media/vaapi_picture_provider.cc (right): https://codereview.chromium.org/490233002/diff/180001/content/common/gpu/medi... content/common/gpu/media/vaapi_picture_provider.cc:89: virtual ~TFPPicture() { provider_->DestroyPicture(this); } On 2014/09/24 11:27:11, Pawel Osciak wrote: > Could we have this class destroy itself (passing x_display and mcc() instead of > provider) instead? Acknowledged. https://codereview.chromium.org/490233002/diff/180001/content/common/gpu/medi... File content/common/gpu/media/video_decode_accelerator_unittest.cc (right): https://codereview.chromium.org/490233002/diff/180001/content/common/gpu/medi... content/common/gpu/media/video_decode_accelerator_unittest.cc:385: static bool MakeDecoderCurrent(scoped_refptr<gfx::GLContext> context) { On 2014/10/08 08:17:21, Pawel Osciak wrote: > On 2014/09/24 11:27:13, Pawel Osciak wrote: > > s/MakeDecoderCurrent/MakeContextCurrent/ > > I think MakeContextCurrent is still a better name? Acknowledged. https://codereview.chromium.org/490233002/diff/220001/content/common/gpu/medi... File content/common/gpu/media/gpu_video_decode_accelerator.cc (right): https://codereview.chromium.org/490233002/diff/220001/content/common/gpu/medi... content/common/gpu/media/gpu_video_decode_accelerator.cc:278: #elif defined(USE_OZONE) On 2014/10/08 08:17:21, Pawel Osciak wrote: > What is happening here now that we go through the case above? Is this dead code? No, as spang and other folks at Intel pointed out, this is a non ChromeOS case (used by people of the Ozone-Wayland project). https://codereview.chromium.org/490233002/diff/220001/content/common/gpu/medi... File content/common/gpu/media/rendering_helper.cc (right): https://codereview.chromium.org/490233002/diff/220001/content/common/gpu/medi... content/common/gpu/media/rendering_helper.cc:87: ui::PlatformWindowState new_state) OVERRIDE{}; On 2014/10/08 08:17:21, Pawel Osciak wrote: > I think we need a space before OVERRIDE (I may be wrong)? > Please make sure to run git cl format on the CL before uploading. I did run git cl format and was a bit surprised that it removed the space... https://codereview.chromium.org/490233002/diff/220001/content/common/gpu/medi... File content/common/gpu/media/vaapi_picture_provider.cc (right): https://codereview.chromium.org/490233002/diff/220001/content/common/gpu/medi... content/common/gpu/media/vaapi_picture_provider.cc:8: #if !defined(USE_X11) On 2014/10/08 08:17:22, Pawel Osciak wrote: > Conditionally compiled headers should go below the regular ones. Acknowledged. https://codereview.chromium.org/490233002/diff/220001/content/common/gpu/medi... File content/common/gpu/media/vaapi_picture_provider.h (right): https://codereview.chromium.org/490233002/diff/220001/content/common/gpu/medi... content/common/gpu/media/vaapi_picture_provider.h:35: }; On 2014/10/08 08:17:22, Pawel Osciak wrote: > Spurious ; Acknowledged. https://codereview.chromium.org/490233002/diff/220001/content/common/gpu/medi... content/common/gpu/media/vaapi_picture_provider.h:39: virtual bool PutSurface(VASurfaceID va_surface_id) = 0; On 2014/10/08 08:17:22, Pawel Osciak wrote: > "Put" may suggest releasing the surface/resources. Perhaps DownloadFromSurface? Acknowledged. https://codereview.chromium.org/490233002/diff/220001/content/common/gpu/medi... content/common/gpu/media/vaapi_picture_provider.h:67: // Notify the picture provider of the picture |size| change. On 2014/10/08 08:17:22, Pawel Osciak wrote: > Please document return value and what happens, it's not obvious. This will disappear (as per one of your other comment). https://codereview.chromium.org/490233002/diff/220001/content/common/gpu/medi... content/common/gpu/media/vaapi_picture_provider.h:75: }; On 2014/10/08 08:17:22, Pawel Osciak wrote: > Please DISALLOW_COPY_AND_ASSIGN here and in Picture classes. Acknowledged. https://codereview.chromium.org/490233002/diff/220001/content/common/gpu/medi... File content/common/gpu/media/vaapi_picture_provider_drm.cc (right): https://codereview.chromium.org/490233002/diff/220001/content/common/gpu/medi... content/common/gpu/media/vaapi_picture_provider_drm.cc:60: CHECK_EQ(glGetError(), GL_NO_ERROR); On 2014/10/08 08:17:22, Pawel Osciak wrote: > I think this is not critical enough to crash the process? Perhaps DCHECK would > be sufficient... Acknowledged. https://codereview.chromium.org/490233002/diff/220001/content/common/gpu/medi... content/common/gpu/media/vaapi_picture_provider_drm.cc:75: factory->CreateNativePixmap(size(), ui::SurfaceFactoryOzone::RGBA_8888); On 2014/10/08 08:17:22, Pawel Osciak wrote: > Check for NULL? Acknowledged. https://codereview.chromium.org/490233002/diff/220001/content/common/gpu/medi... content/common/gpu/media/vaapi_picture_provider_drm.cc:76: unsigned long buffer_fd = pixmap_->GetDmaBufFd(); On 2014/10/08 08:17:22, Pawel Osciak wrote: > fd is an int... We should also check for -1. Acknowledged. https://codereview.chromium.org/490233002/diff/220001/content/common/gpu/medi... content/common/gpu/media/vaapi_picture_provider_drm.cc:160: DrmPicture* drm_picture = new DrmPicture(this, On 2014/10/08 08:17:22, Pawel Osciak wrote: > Could we create a scoped_ptr directly here? > scoped_ptr<Picture> picture(new DrmPicture(...)). The problem is that I can't access the Initialize method of DrmPicture anymore if I do that. (The reason the Initialize is not part of VaapiPictureProvider::Picture is that the initialization is slightly different in X11 and DRM cases.) https://codereview.chromium.org/490233002/diff/220001/content/common/gpu/medi... content/common/gpu/media/vaapi_picture_provider_drm.cc:166: scoped_ptr<VaapiPictureProvider::Picture> picture(drm_picture); On 2014/10/08 08:17:22, Pawel Osciak wrote: > Do we need the qualifier here if we are child of VaapiPictureProvider? I'll check, thanks. https://codereview.chromium.org/490233002/diff/220001/content/common/gpu/medi... content/common/gpu/media/vaapi_picture_provider_drm.cc:217: bool DrmVaapiPictureProvider::SetCodedSurfacesSize(const gfx::Size& size) { On 2014/10/08 08:17:22, Pawel Osciak wrote: > I realized, could we just do this on PutSurfaceIntoPicture? Check if size passed > to it is different than previous size and if so, reinitialize? Acknowledged. https://codereview.chromium.org/490233002/diff/220001/content/common/gpu/medi... File content/common/gpu/media/vaapi_picture_provider_drm.h (right): https://codereview.chromium.org/490233002/diff/220001/content/common/gpu/medi... content/common/gpu/media/vaapi_picture_provider_drm.h:35: bool PutSurfaceIntoPicture(VASurfaceID va_surface_id_src, On 2014/10/08 08:17:22, Pawel Osciak wrote: > This should be private? With VPP functions into the VaapiWrapper, this will disappear. https://codereview.chromium.org/490233002/diff/220001/content/common/gpu/medi... File content/common/gpu/media/vaapi_picture_provider_x11.cc (right): https://codereview.chromium.org/490233002/diff/220001/content/common/gpu/medi... content/common/gpu/media/vaapi_picture_provider_x11.cc:94: LOG_VA_ERROR_AND_RETURN(vaPutSurface(va_display_, On 2014/10/08 08:17:22, Pawel Osciak wrote: > Could we please keep using the VaapiWrapper instance that VAVDA uses, passing > the wrapper instead of va display into the provider? Especially since it's the > owner of VASurface. Acknowledged. https://codereview.chromium.org/490233002/diff/220001/content/common/gpu/medi... File content/common/gpu/media/vaapi_video_decode_accelerator.cc (right): https://codereview.chromium.org/490233002/diff/220001/content/common/gpu/medi... content/common/gpu/media/vaapi_video_decode_accelerator.cc:91: // Make sure pictures are destroyed before VaapiWrapper is, so the On 2014/10/08 08:17:22, Pawel Osciak wrote: > If we keep a refcount to the wrapper in provider, this should not be needed. We can either use refcounting or move the pictures further down the private members in the header. I feel refcounting is safer because relying on the destruction order is not so obvious (at least to me). If you have a preference let me know. https://codereview.chromium.org/490233002/diff/220001/content/common/gpu/medi... File content/common/gpu/media/vaapi_video_decode_accelerator.h (right): https://codereview.chromium.org/490233002/diff/220001/content/common/gpu/medi... content/common/gpu/media/vaapi_video_decode_accelerator.h:151: // Client-provided X/GLX state. On 2014/10/08 08:17:22, Pawel Osciak wrote: > Stale comment. Acknowledged. https://codereview.chromium.org/490233002/diff/220001/content/common/gpu/medi... content/common/gpu/media/vaapi_video_decode_accelerator.h:246: scoped_ptr<VaapiPictureProvider> vaapi_picture_provider_; On 2014/10/08 08:17:22, Pawel Osciak wrote: > We should add a comment that it is important this outlives wrapper, and has to > stay defined after it for proper destruction order. It would be good to have the pictures down here too, to ensure they're destroyed before the VaapiWrapper https://codereview.chromium.org/490233002/diff/220001/content/common/gpu/medi... File content/common/gpu/media/vaapi_wrapper.h (right): https://codereview.chromium.org/490233002/diff/220001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.h:129: // Returns the initialized VADisplay On 2014/10/08 08:17:22, Pawel Osciak wrote: > I feel really uncomfortable about this. We should either reuse the wrapper > instance, or wrap and refcount the display (but that would be an overkill, so I > prefer the former). I will implement the VPP functions into the VaapiWrapper and remove this. https://codereview.chromium.org/490233002/diff/220001/content/common/gpu/medi... File content/common/gpu/media/video_decode_accelerator_unittest.cc (right): https://codereview.chromium.org/490233002/diff/220001/content/common/gpu/medi... content/common/gpu/media/video_decode_accelerator_unittest.cc:385: static bool MakeDecoderCurrent(scoped_refptr<gfx::GLContext> context) { On 2014/10/08 08:17:22, Pawel Osciak wrote: > MakeContextCurrent Acknowledged. https://codereview.chromium.org/490233002/diff/220001/content/common/gpu/medi... content/common/gpu/media/video_decode_accelerator_unittest.cc:877: #if defined(OS_WIN) || defined(USE_OZONE) On 2014/10/08 08:17:22, Pawel Osciak wrote: > Is this also required for ozone? The comment below seems to be very > Windows-specific. Yes, Ozone needs have a UI type thread too. I will modify the comment. https://codereview.chromium.org/490233002/diff/220001/content/common/gpu/medi... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/490233002/diff/220001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:1035: ui::OzonePlatform::InitializeForUI(); On 2014/10/08 08:17:23, Pawel Osciak wrote: > There is no rendering in this test. Do we need these? Not sure what you call rendering, but this test paints using GL. The last test also does a glReadPixels to verify that the rendered surface matches a md5 hash in media/test/data/test-25fps.h264.md5 https://codereview.chromium.org/490233002/diff/220001/ui/ozone/platform/dri/g... File ui/ozone/platform/dri/gbm_buffer.cc (right): https://codereview.chromium.org/490233002/diff/220001/ui/ozone/platform/dri/g... ui/ozone/platform/dri/gbm_buffer.cc:74: return gbm_bo_get_fd(buffer_->bo()); On 2014/10/08 08:17:23, Pawel Osciak wrote: > I think we should be closing the fd somewhere once we are done with it? When we give the fd to LibVA, it is wrapped using drm_intel_bo_gem_create_from_prime : http://cgit.freedesktop.org/mesa/drm/tree/intel/intel_bufmgr.h?id=86b37c61c78... Libdrm then does refcounting on the drm_intel_bo objects, and ensure the fd is closed when unused. As mentionned https://codereview.chromium.org/490233002/#msg41 , we need some patches in libdrm (or upgrade to 2.4.57 to have drm BOs properly managed between GL and LibVA.
https://codereview.chromium.org/490233002/diff/180001/content/common/gpu/medi... File content/common/gpu/media/vaapi_video_decode_accelerator.h (right): https://codereview.chromium.org/490233002/diff/180001/content/common/gpu/medi... content/common/gpu/media/vaapi_video_decode_accelerator.h:198: typedef base::ScopedPtrHashMap<int32, VaapiPictureProvider::Picture> Pictures; On 2014/10/08 08:17:21, Pawel Osciak wrote: > On 2014/10/02 14:14:23, llandwerlin wrote: > > On 2014/09/24 11:27:12, Pawel Osciak wrote: > > > Why do we need a hashmap? > > > > I couldn't find a data structure with the same properties as std::map that > would > > also call delete on the pointer stored. > > Would std::map<int32, scoped_ptr<VaapiPictureProvider::Picture> > be ok? > > linked_ptr currently used does delete... Removing this change. https://codereview.chromium.org/490233002/diff/180001/content/common/gpu/medi... File content/common/gpu/media/video_decode_accelerator_unittest.cc (right): https://codereview.chromium.org/490233002/diff/180001/content/common/gpu/medi... content/common/gpu/media/video_decode_accelerator_unittest.cc:384: #if defined(OS_CHROMEOS) && defined(ARCH_CPU_X86_FAMILY) On 2014/10/08 08:17:21, Pawel Osciak wrote: > On 2014/09/24 11:27:13, Pawel Osciak wrote: > > Why were we ok without this before? > > Question still stands please. Removed. https://codereview.chromium.org/490233002/diff/220001/content/common/gpu/medi... File content/common/gpu/media/vaapi_picture_provider_drm.cc (right): https://codereview.chromium.org/490233002/diff/220001/content/common/gpu/medi... content/common/gpu/media/vaapi_picture_provider_drm.cc:166: scoped_ptr<VaapiPictureProvider::Picture> picture(drm_picture); On 2014/10/08 09:31:18, llandwerlin wrote: > On 2014/10/08 08:17:22, Pawel Osciak wrote: > > Do we need the qualifier here if we are child of VaapiPictureProvider? > > I'll check, thanks. Yep, we do... https://codereview.chromium.org/490233002/diff/220001/content/common/gpu/medi... File content/common/gpu/media/vaapi_wrapper.cc (right): https://codereview.chromium.org/490233002/diff/220001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.cc:158: va_display = vaGetDisplayDRM(factory->GetDrmFd()); On 2014/10/08 08:17:22, Pawel Osciak wrote: > Do we need to close the fd somewhere? I don't think we need to do anything here. The file descriptor is opened by the Ozone platform code and given to the SurfaceFactoryOzone. It will be closed when the window system is shutdown. At this point all video decoders should have been shutdown already.
After the rebase, some tests are now failing. The tests failing seems to be related to frame rate limitation (tests are expecting 60fps, but the swapbuffer seems to be limited at 30fps), I will try to investigate the problem but this seems unrelated to this CL.
Getting close, thank you for all the changes. I wish there was a way not to keep size in VASurface, but I don't see a way to not do this yet... https://chromiumcodereview.appspot.com/490233002/diff/220001/content/common/g... File content/common/gpu/media/vaapi_picture_provider_drm.cc (right): https://chromiumcodereview.appspot.com/490233002/diff/220001/content/common/g... content/common/gpu/media/vaapi_picture_provider_drm.cc:160: DrmPicture* drm_picture = new DrmPicture(this, On 2014/10/08 09:31:18, llandwerlin wrote: > On 2014/10/08 08:17:22, Pawel Osciak wrote: > > Could we create a scoped_ptr directly here? > > scoped_ptr<Picture> picture(new DrmPicture(...)). > > The problem is that I can't access the Initialize method of DrmPicture anymore > if I do that. > (The reason the Initialize is not part of VaapiPictureProvider::Picture is that > the initialization is slightly different in X11 and DRM cases.) Could you pass fb_config_ to the constructor of TFPPicture? Then Initialize() would be the same (0 arguments) and could be virtual? And you could even call Initialize from outside Create. https://chromiumcodereview.appspot.com/490233002/diff/220001/content/common/g... File content/common/gpu/media/vaapi_video_decode_accelerator.cc (right): https://chromiumcodereview.appspot.com/490233002/diff/220001/content/common/g... content/common/gpu/media/vaapi_video_decode_accelerator.cc:91: // Make sure pictures are destroyed before VaapiWrapper is, so the On 2014/10/08 09:31:18, llandwerlin wrote: > On 2014/10/08 08:17:22, Pawel Osciak wrote: > > If we keep a refcount to the wrapper in provider, this should not be needed. > > We can either use refcounting or move the pictures further down the private > members in the header. > I feel refcounting is safer because relying on the destruction order is not so > obvious (at least to me). > If you have a preference let me know. Refcounting of course; now we have a ref to wrapper on each picture so no issues anymore I believe? https://chromiumcodereview.appspot.com/490233002/diff/220001/content/common/g... File content/common/gpu/media/vaapi_video_decode_accelerator.h (right): https://chromiumcodereview.appspot.com/490233002/diff/220001/content/common/g... content/common/gpu/media/vaapi_video_decode_accelerator.h:246: scoped_ptr<VaapiPictureProvider> vaapi_picture_provider_; On 2014/10/08 09:31:18, llandwerlin wrote: > On 2014/10/08 08:17:22, Pawel Osciak wrote: > > We should add a comment that it is important this outlives wrapper, and has to > > stay defined after it for proper destruction order. > > It would be good to have the pictures down here too, to ensure they're destroyed > before the VaapiWrapper Providers have a ref to the wrapper now, so this is not a problem anymore? https://chromiumcodereview.appspot.com/490233002/diff/220001/content/common/g... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://chromiumcodereview.appspot.com/490233002/diff/220001/content/common/g... content/common/gpu/media/video_encode_accelerator_unittest.cc:1035: ui::OzonePlatform::InitializeForUI(); On 2014/10/08 09:31:18, llandwerlin wrote: > On 2014/10/08 08:17:23, Pawel Osciak wrote: > > There is no rendering in this test. Do we need these? > > Not sure what you call rendering, but this test paints using GL. > The last test also does a glReadPixels to verify that the rendered surface > matches a md5 hash in media/test/data/test-25fps.h264.md5 This is the encoder test, I think you are mistaking it with the decoder one. This one does not do any of these... https://chromiumcodereview.appspot.com/490233002/diff/220001/ui/ozone/platfor... File ui/ozone/platform/dri/gbm_buffer.cc (right): https://chromiumcodereview.appspot.com/490233002/diff/220001/ui/ozone/platfor... ui/ozone/platform/dri/gbm_buffer.cc:74: return gbm_bo_get_fd(buffer_->bo()); On 2014/10/08 09:31:18, llandwerlin wrote: > On 2014/10/08 08:17:23, Pawel Osciak wrote: > > I think we should be closing the fd somewhere once we are done with it? > > When we give the fd to LibVA, it is wrapped using > drm_intel_bo_gem_create_from_prime : > http://cgit.freedesktop.org/mesa/drm/tree/intel/intel_bufmgr.h?id=86b37c61c78... > > Libdrm then does refcounting on the drm_intel_bo objects, and ensure the fd is > closed when unused. > > As mentionned https://codereview.chromium.org/490233002/#msg41 , we need some > patches in libdrm (or upgrade to 2.4.57 to have drm BOs properly managed between > GL and LibVA. This ends up in DRM_IOCTL_PRIME_FD_TO_HANDLE and that calls get_dma_buf() which increases the refcount on associated file. Also, the documentation for gbm_bo_get_fd() says we need to be closing it. And the users such as Wayland are doing this (e.g. http://people.freedesktop.org/~chadversary/mesa/doxygen/kevin-rogovin/dd/d68/...). https://chromiumcodereview.appspot.com/490233002/diff/300001/content/common/g... File content/common/gpu/media/vaapi_picture_provider.h (right): https://chromiumcodereview.appspot.com/490233002/diff/300001/content/common/g... content/common/gpu/media/vaapi_picture_provider.h:65: // |va_display|. |gl_context| and |make_context_current| will be Stale comment. https://chromiumcodereview.appspot.com/490233002/diff/300001/content/common/g... File content/common/gpu/media/vaapi_picture_provider_drm.cc (right): https://chromiumcodereview.appspot.com/490233002/diff/300001/content/common/g... content/common/gpu/media/vaapi_picture_provider_drm.cc:53: vaapi_wrapper_->DestroyOutputSurface(va_surface_); What is responsible for closing the fd? Will it be done by the libva driver? Is it taking over ownership after a surface is created from it? https://chromiumcodereview.appspot.com/490233002/diff/300001/content/common/g... content/common/gpu/media/vaapi_picture_provider_drm.cc:68: unsigned long buffer_fd = pixmap_->GetDmaBufFd(); fd is an int. https://chromiumcodereview.appspot.com/490233002/diff/300001/content/common/g... content/common/gpu/media/vaapi_picture_provider_drm.cc:99: return false; What happens to fd? I think we need to close it? Documentation for gbm_bo_get_fd() says we are supposed to. https://chromiumcodereview.appspot.com/490233002/diff/300001/content/common/g... content/common/gpu/media/vaapi_picture_provider_drm.cc:116: va_surface_id, surface_size, va_surface_, size())) Indent is wrong. https://chromiumcodereview.appspot.com/490233002/diff/300001/content/common/g... content/common/gpu/media/vaapi_picture_provider_drm.cc:129: VASurfaceID va_surface_; s/va_surface/va_surface_id_ https://chromiumcodereview.appspot.com/490233002/diff/300001/content/common/g... File content/common/gpu/media/vaapi_picture_provider_x11.cc (right): https://chromiumcodereview.appspot.com/490233002/diff/300001/content/common/g... content/common/gpu/media/vaapi_picture_provider_x11.cc:13: class TFPPicture : public VaapiPictureProvider::Picture { Please do not define nontrivial methods in the body of a class definition. Here and everywhere else. https://chromiumcodereview.appspot.com/490233002/diff/300001/content/common/g... File content/common/gpu/media/vaapi_picture_provider_x11.h (right): https://chromiumcodereview.appspot.com/490233002/diff/300001/content/common/g... content/common/gpu/media/vaapi_picture_provider_x11.h:14: #include "content/common/gpu/media/vaapi_wrapper.h" Would declaration be enough like in drm case? https://chromiumcodereview.appspot.com/490233002/diff/300001/content/common/g... File content/common/gpu/media/vaapi_wrapper.cc (right): https://chromiumcodereview.appspot.com/490233002/diff/300001/content/common/g... content/common/gpu/media/vaapi_wrapper.cc:706: VARectangle input_region, output_region; Please define variables as late as possible. Here and everywhere else. https://chromiumcodereview.appspot.com/490233002/diff/300001/content/common/g... content/common/gpu/media/vaapi_wrapper.cc:716: base::AutoLock auto_lock(va_lock_); You need the lock for the duration of the entire method, otherwise someone can reinitialize vpp again before you reacquire the lock here. https://chromiumcodereview.appspot.com/490233002/diff/300001/content/common/g... content/common/gpu/media/vaapi_wrapper.cc:718: VA_SUCCESS_OR_RETURN(vaSyncSurface(va_display_, va_surface_id_src), I think we could move this even lower to give it more time to process before we block on this, even if just a little bit? Do we actually have to do this? I thought unless we had to touch with CPU, this kind of synchronization would be handler in the driver's pipeline? https://chromiumcodereview.appspot.com/490233002/diff/300001/content/common/g... content/common/gpu/media/vaapi_wrapper.cc:723: vaMapBuffer(va_display_, va_vpp_buffer_, (void**)&pipeline_param), Please no c-style casts. reinterpret_cast. https://chromiumcodereview.appspot.com/490233002/diff/300001/content/common/g... File content/common/gpu/media/vaapi_wrapper.h (right): https://chromiumcodereview.appspot.com/490233002/diff/300001/content/common/g... content/common/gpu/media/vaapi_wrapper.h:72: // Create a VASurface of |format|, |size| and using attributes s/format/va_format/ https://chromiumcodereview.appspot.com/490233002/diff/300001/content/common/g... content/common/gpu/media/vaapi_wrapper.h:73: // |attribs|. The Client must call DestroyOutputSurface() to destroy where |num_va_attribs| is the size of |va_attribs|. https://chromiumcodereview.appspot.com/490233002/diff/300001/content/common/g... content/common/gpu/media/vaapi_wrapper.h:74: // the created surface. What if we returned a VASurface instead and have DestroyOutputSurface() as its release_cb(), would that work? https://chromiumcodereview.appspot.com/490233002/diff/300001/content/common/g... content/common/gpu/media/vaapi_wrapper.h:75: bool CreateOutputSurface(unsigned int va_format, I'd call it CreateUnmanagedSurface, or CreateUnownedSurface, and clearly state in the documentation how this differs from CreateSurfaces. https://chromiumcodereview.appspot.com/490233002/diff/300001/content/common/g... content/common/gpu/media/vaapi_wrapper.h:79: VASurfaceID* va_surface); Please document va_surface too. Also please always call *ID *_id, i.e. va_surface_id, here and everywhere. https://chromiumcodereview.appspot.com/490233002/diff/300001/content/common/g... content/common/gpu/media/vaapi_wrapper.h:147: // output picture format (RGBA) Full stop at the end of sentence please. Also please mention that this may scale. https://chromiumcodereview.appspot.com/490233002/diff/300001/content/common/g... content/common/gpu/media/vaapi_wrapper.h:148: bool PutSurfaceIntoSurface(VASurfaceID va_surface_id_src, PutSurfaceIntoPixmap() is a download, but this is not. I'd suggest something like ConvertSurface() perhaps? https://chromiumcodereview.appspot.com/490233002/diff/300001/content/common/g... content/common/gpu/media/vaapi_wrapper.h:171: // the decoder output pictures. I think we don't want to mention decoder here. We could even use this for something else not related to decode. Also, please say that this is going to be the source size. Moreover, could we make this universal and pass source format as well? I'm forgetting right now, is the source format changeable as well? https://chromiumcodereview.appspot.com/490233002/diff/300001/content/common/g... content/common/gpu/media/vaapi_wrapper.h:174: // Deinitialize the video post processing context Please full stop at the end of sentences. https://chromiumcodereview.appspot.com/490233002/diff/300001/content/common/g... content/common/gpu/media/vaapi_wrapper.h:220: VAConfigID va_vpp_config_; Please add _id at the end of each of these. https://chromiumcodereview.appspot.com/490233002/diff/300001/content/common/g... content/common/gpu/media/vaapi_wrapper.h:224: // Cache of the size of the VPP input pictures "Input surface size, to which VPP is currently configured." https://chromiumcodereview.appspot.com/490233002/diff/300001/content/common/g... File content/common/gpu/media/video_decode_accelerator_unittest.cc (right): https://chromiumcodereview.appspot.com/490233002/diff/300001/content/common/g... content/common/gpu/media/video_decode_accelerator_unittest.cc:872: // On Ozone, the backend initialize the event system using a UI s/initialize/initializes/
Thanks a lot for the review, it's been really helpful to figure out some tricky problems. https://codereview.chromium.org/490233002/diff/220001/content/common/gpu/medi... File content/common/gpu/media/vaapi_picture_provider_drm.cc (right): https://codereview.chromium.org/490233002/diff/220001/content/common/gpu/medi... content/common/gpu/media/vaapi_picture_provider_drm.cc:160: DrmPicture* drm_picture = new DrmPicture(this, On 2014/10/26 13:06:46, Pawel Osciak wrote: > On 2014/10/08 09:31:18, llandwerlin wrote: > > On 2014/10/08 08:17:22, Pawel Osciak wrote: > > > Could we create a scoped_ptr directly here? > > > scoped_ptr<Picture> picture(new DrmPicture(...)). > > > > The problem is that I can't access the Initialize method of DrmPicture anymore > > if I do that. > > (The reason the Initialize is not part of VaapiPictureProvider::Picture is > that > > the initialization is slightly different in X11 and DRM cases.) > > Could you pass fb_config_ to the constructor of TFPPicture? Then Initialize() > would be the same (0 arguments) and could be virtual? > And you could even call Initialize from outside Create. Acknowledged. https://codereview.chromium.org/490233002/diff/220001/content/common/gpu/medi... File content/common/gpu/media/vaapi_video_decode_accelerator.cc (right): https://codereview.chromium.org/490233002/diff/220001/content/common/gpu/medi... content/common/gpu/media/vaapi_video_decode_accelerator.cc:91: // Make sure pictures are destroyed before VaapiWrapper is, so the On 2014/10/26 13:06:46, Pawel Osciak wrote: > On 2014/10/08 09:31:18, llandwerlin wrote: > > On 2014/10/08 08:17:22, Pawel Osciak wrote: > > > If we keep a refcount to the wrapper in provider, this should not be needed. > > > > We can either use refcounting or move the pictures further down the private > > members in the header. > > I feel refcounting is safer because relying on the destruction order is not so > > obvious (at least to me). > > If you have a preference let me know. > > Refcounting of course; now we have a ref to wrapper on each picture so no issues > anymore I believe? Acknowledged. https://codereview.chromium.org/490233002/diff/220001/content/common/gpu/medi... File content/common/gpu/media/vaapi_video_decode_accelerator.h (right): https://codereview.chromium.org/490233002/diff/220001/content/common/gpu/medi... content/common/gpu/media/vaapi_video_decode_accelerator.h:246: scoped_ptr<VaapiPictureProvider> vaapi_picture_provider_; On 2014/10/26 13:06:46, Pawel Osciak wrote: > On 2014/10/08 09:31:18, llandwerlin wrote: > > On 2014/10/08 08:17:22, Pawel Osciak wrote: > > > We should add a comment that it is important this outlives wrapper, and has > to > > > stay defined after it for proper destruction order. > > > > It would be good to have the pictures down here too, to ensure they're > destroyed > > before the VaapiWrapper > > Providers have a ref to the wrapper now, so this is not a problem anymore? Acknowledged. https://codereview.chromium.org/490233002/diff/220001/content/common/gpu/medi... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/490233002/diff/220001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:1035: ui::OzonePlatform::InitializeForUI(); On 2014/10/26 13:06:46, Pawel Osciak wrote: > On 2014/10/08 09:31:18, llandwerlin wrote: > > On 2014/10/08 08:17:23, Pawel Osciak wrote: > > > There is no rendering in this test. Do we need these? > > > > Not sure what you call rendering, but this test paints using GL. > > The last test also does a glReadPixels to verify that the rendered surface > > matches a md5 hash in media/test/data/test-25fps.h264.md5 > > This is the encoder test, I think you are mistaking it with the decoder one. > This one does not do any of these... Ah right, I misread the file name. Though I need to initialize GL when using the gbm backend, we initialize libva using the drm fd given by gbm_device_get_fd(). I guess this won't be a problem anymore when minigbm is used as it won't require GL to be initialized. https://codereview.chromium.org/490233002/diff/220001/ui/ozone/platform/dri/g... File ui/ozone/platform/dri/gbm_buffer.cc (right): https://codereview.chromium.org/490233002/diff/220001/ui/ozone/platform/dri/g... ui/ozone/platform/dri/gbm_buffer.cc:74: return gbm_bo_get_fd(buffer_->bo()); On 2014/10/26 13:06:46, Pawel Osciak wrote: > On 2014/10/08 09:31:18, llandwerlin wrote: > > On 2014/10/08 08:17:23, Pawel Osciak wrote: > > > I think we should be closing the fd somewhere once we are done with it? > > > > When we give the fd to LibVA, it is wrapped using > > drm_intel_bo_gem_create_from_prime : > > > http://cgit.freedesktop.org/mesa/drm/tree/intel/intel_bufmgr.h?id=86b37c61c78... > > > > Libdrm then does refcounting on the drm_intel_bo objects, and ensure the fd is > > closed when unused. > > > > As mentionned https://codereview.chromium.org/490233002/#msg41 , we need some > > patches in libdrm (or upgrade to 2.4.57 to have drm BOs properly managed > between > > GL and LibVA. > > This ends up in DRM_IOCTL_PRIME_FD_TO_HANDLE and that calls get_dma_buf() which > increases the refcount on associated file. Also, the documentation for > gbm_bo_get_fd() says we need to be closing it. And the users such as Wayland are > doing this (e.g. > http://people.freedesktop.org/~chadversary/mesa/doxygen/kevin-rogovin/dd/d68/...). Right, thanks, we effectively leaking the fd here. I will add a comment in the drm picture provider to explain how all this works. https://codereview.chromium.org/490233002/diff/300001/content/common/gpu/medi... File content/common/gpu/media/vaapi_picture_provider.h (right): https://codereview.chromium.org/490233002/diff/300001/content/common/gpu/medi... content/common/gpu/media/vaapi_picture_provider.h:65: // |va_display|. |gl_context| and |make_context_current| will be On 2014/10/26 13:06:46, Pawel Osciak wrote: > Stale comment. Acknowledged. https://codereview.chromium.org/490233002/diff/300001/content/common/gpu/medi... File content/common/gpu/media/vaapi_picture_provider_drm.cc (right): https://codereview.chromium.org/490233002/diff/300001/content/common/gpu/medi... content/common/gpu/media/vaapi_picture_provider_drm.cc:53: vaapi_wrapper_->DestroyOutputSurface(va_surface_); On 2014/10/26 13:06:46, Pawel Osciak wrote: > What is responsible for closing the fd? Will it be done by the libva driver? Is > it taking over ownership after a surface is created from it? Closing the fd here now. https://codereview.chromium.org/490233002/diff/300001/content/common/gpu/medi... content/common/gpu/media/vaapi_picture_provider_drm.cc:68: unsigned long buffer_fd = pixmap_->GetDmaBufFd(); On 2014/10/26 13:06:46, Pawel Osciak wrote: > fd is an int. Acknowledged. https://codereview.chromium.org/490233002/diff/300001/content/common/gpu/medi... content/common/gpu/media/vaapi_picture_provider_drm.cc:99: return false; On 2014/10/26 13:06:46, Pawel Osciak wrote: > What happens to fd? I think we need to close it? > Documentation for gbm_bo_get_fd() says we are supposed to. Closing in the destructor now. https://codereview.chromium.org/490233002/diff/300001/content/common/gpu/medi... content/common/gpu/media/vaapi_picture_provider_drm.cc:116: va_surface_id, surface_size, va_surface_, size())) On 2014/10/26 13:06:46, Pawel Osciak wrote: > Indent is wrong. I'm sure I run cl format... Will rerun. https://codereview.chromium.org/490233002/diff/300001/content/common/gpu/medi... content/common/gpu/media/vaapi_picture_provider_drm.cc:129: VASurfaceID va_surface_; On 2014/10/26 13:06:46, Pawel Osciak wrote: > s/va_surface/va_surface_id_ Acknowledged. https://codereview.chromium.org/490233002/diff/300001/content/common/gpu/medi... File content/common/gpu/media/vaapi_picture_provider_x11.cc (right): https://codereview.chromium.org/490233002/diff/300001/content/common/gpu/medi... content/common/gpu/media/vaapi_picture_provider_x11.cc:13: class TFPPicture : public VaapiPictureProvider::Picture { On 2014/10/26 13:06:46, Pawel Osciak wrote: > Please do not define nontrivial methods in the body of a class definition. Here > and everywhere else. Acknowledged. https://codereview.chromium.org/490233002/diff/300001/content/common/gpu/medi... File content/common/gpu/media/vaapi_picture_provider_x11.h (right): https://codereview.chromium.org/490233002/diff/300001/content/common/gpu/medi... content/common/gpu/media/vaapi_picture_provider_x11.h:14: #include "content/common/gpu/media/vaapi_wrapper.h" On 2014/10/26 13:06:47, Pawel Osciak wrote: > Would declaration be enough like in drm case? Sadly since I've moved the vaDestroySurfaces() into VaapiWrapper, the compiler needs the declaration to find the methods. https://codereview.chromium.org/490233002/diff/300001/content/common/gpu/medi... File content/common/gpu/media/vaapi_wrapper.cc (right): https://codereview.chromium.org/490233002/diff/300001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.cc:706: VARectangle input_region, output_region; On 2014/10/26 13:06:47, Pawel Osciak wrote: > Please define variables as late as possible. Here and everywhere else. Acknowledged. https://codereview.chromium.org/490233002/diff/300001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.cc:716: base::AutoLock auto_lock(va_lock_); On 2014/10/26 13:06:47, Pawel Osciak wrote: > You need the lock for the duration of the entire method, otherwise someone can > reinitialize vpp again before you reacquire the lock here. Acknowledged. https://codereview.chromium.org/490233002/diff/300001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.cc:718: VA_SUCCESS_OR_RETURN(vaSyncSurface(va_display_, va_surface_id_src), On 2014/10/26 13:06:47, Pawel Osciak wrote: > I think we could move this even lower to give it more time to process before we > block on this, even if just a little bit? > Do we actually have to do this? I thought unless we had to touch with CPU, this > kind of synchronization would be handler in the driver's pipeline? That's true, since it's all coming from the GPU, we don't need to synchronize here. Removing this. Although in the future this could be used as input for the encoder with a input buffer potentially in user space (but I guess will either have another method or add a parameter). https://codereview.chromium.org/490233002/diff/300001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.cc:723: vaMapBuffer(va_display_, va_vpp_buffer_, (void**)&pipeline_param), On 2014/10/26 13:06:47, Pawel Osciak wrote: > Please no c-style casts. reinterpret_cast. Acknowledged. https://codereview.chromium.org/490233002/diff/300001/content/common/gpu/medi... File content/common/gpu/media/vaapi_wrapper.h (right): https://codereview.chromium.org/490233002/diff/300001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.h:72: // Create a VASurface of |format|, |size| and using attributes On 2014/10/26 13:06:48, Pawel Osciak wrote: > s/format/va_format/ Acknowledged. https://codereview.chromium.org/490233002/diff/300001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.h:73: // |attribs|. The Client must call DestroyOutputSurface() to destroy On 2014/10/26 13:06:48, Pawel Osciak wrote: > where |num_va_attribs| is the size of |va_attribs|. Acknowledged. https://codereview.chromium.org/490233002/diff/300001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.h:74: // the created surface. On 2014/10/26 13:06:48, Pawel Osciak wrote: > What if we returned a VASurface instead and have DestroyOutputSurface() as its > release_cb(), would that work? Yes, doing this. https://codereview.chromium.org/490233002/diff/300001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.h:75: bool CreateOutputSurface(unsigned int va_format, On 2014/10/26 13:06:48, Pawel Osciak wrote: > I'd call it CreateUnmanagedSurface, or CreateUnownedSurface, and clearly state > in the documentation how this differs from CreateSurfaces. Acknowledged. https://codereview.chromium.org/490233002/diff/300001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.h:79: VASurfaceID* va_surface); On 2014/10/26 13:06:48, Pawel Osciak wrote: > Please document va_surface too. > Also please always call *ID *_id, i.e. va_surface_id, here and everywhere. Acknowledged. https://codereview.chromium.org/490233002/diff/300001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.h:147: // output picture format (RGBA) On 2014/10/26 13:06:48, Pawel Osciak wrote: > Full stop at the end of sentence please. Also please mention that this may > scale. Acknowledged. https://codereview.chromium.org/490233002/diff/300001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.h:148: bool PutSurfaceIntoSurface(VASurfaceID va_surface_id_src, On 2014/10/26 13:06:48, Pawel Osciak wrote: > PutSurfaceIntoPixmap() is a download, but this is not. I'd suggest something > like ConvertSurface() perhaps? Went with BlitSurface(). https://codereview.chromium.org/490233002/diff/300001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.h:171: // the decoder output pictures. On 2014/10/26 13:06:48, Pawel Osciak wrote: > I think we don't want to mention decoder here. We could even use this for > something else not related to decode. > Also, please say that this is going to be the source size. > Moreover, could we make this universal and pass source format as well? I'm > forgetting right now, is the source format changeable as well? Actually looking into the code of the va driver on Intel, the size given to initialize the context is irrelevant for the post processor. I'll just drop the size. https://codereview.chromium.org/490233002/diff/300001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.h:174: // Deinitialize the video post processing context On 2014/10/26 13:06:48, Pawel Osciak wrote: > Please full stop at the end of sentences. Acknowledged. https://codereview.chromium.org/490233002/diff/300001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.h:220: VAConfigID va_vpp_config_; On 2014/10/26 13:06:48, Pawel Osciak wrote: > Please add _id at the end of each of these. Acknowledged. https://codereview.chromium.org/490233002/diff/300001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.h:224: // Cache of the size of the VPP input pictures On 2014/10/26 13:06:48, Pawel Osciak wrote: > "Input surface size, to which VPP is currently configured." Removed. https://codereview.chromium.org/490233002/diff/300001/content/common/gpu/medi... File content/common/gpu/media/video_decode_accelerator_unittest.cc (right): https://codereview.chromium.org/490233002/diff/300001/content/common/gpu/medi... content/common/gpu/media/video_decode_accelerator_unittest.cc:872: // On Ozone, the backend initialize the event system using a UI On 2014/10/26 13:06:48, Pawel Osciak wrote: > s/initialize/initializes/ Acknowledged.
After this last rebase, the tests are passing again. Looks like someone fixed the SwapBuffers() problems I experienced in the last iteration. It might be worth reviewing again the ozone part. PTAL, Thanks.
On 2014/10/29 13:55:16, llandwerlin wrote: > After this last rebase, the tests are passing again. > Looks like someone fixed the SwapBuffers() problems I experienced in the last > iteration. > Also, related fix in Mesa is committed now http://cgit.freedesktop.org/mesa/mesa/commit/?id=d175e7c16b4de1a4231b806dad3e...
https://codereview.chromium.org/490233002/diff/360001/content/content_common.... File content/content_common.gypi (right): https://codereview.chromium.org/490233002/diff/360001/content/content_common.... content/content_common.gypi:775: ['target_arch != "arm" and chromeos == 1', { here we need to opt out also platforms that don't provide hw acceleration, e.g. Ozone DRI, testing etc
kalyan.kondapally@intel.com changed reviewers: + kalyan.kondapally@intel.com
https://chromiumcodereview.appspot.com/490233002/diff/220001/ui/ozone/platfor... File ui/ozone/platform/dri/gbm_buffer.cc (right): https://chromiumcodereview.appspot.com/490233002/diff/220001/ui/ozone/platfor... ui/ozone/platform/dri/gbm_buffer.cc:74: return gbm_bo_get_fd(buffer_->bo()); On 2014/10/29 13:52:47, llandwerlin wrote: > On 2014/10/26 13:06:46, Pawel Osciak wrote: > > On 2014/10/08 09:31:18, llandwerlin wrote: > > > On 2014/10/08 08:17:23, Pawel Osciak wrote: > > > > I think we should be closing the fd somewhere once we are done with it? > > > > > > When we give the fd to LibVA, it is wrapped using > > > drm_intel_bo_gem_create_from_prime : > > > > > > http://cgit.freedesktop.org/mesa/drm/tree/intel/intel_bufmgr.h?id=86b37c61c78... > > > > > > Libdrm then does refcounting on the drm_intel_bo objects, and ensure the fd > is > > > closed when unused. > > > > > > As mentionned https://codereview.chromium.org/490233002/#msg41 , we need > some > > > patches in libdrm (or upgrade to 2.4.57 to have drm BOs properly managed > > between > > > GL and LibVA. > > > > This ends up in DRM_IOCTL_PRIME_FD_TO_HANDLE and that calls get_dma_buf() > which > > increases the refcount on associated file. Also, the documentation for > > gbm_bo_get_fd() says we need to be closing it. And the users such as Wayland > are > > doing this (e.g. > > > http://people.freedesktop.org/~chadversary/mesa/doxygen/kevin-rogovin/dd/d68/...). > > Right, thanks, we effectively leaking the fd here. I will add a comment in the > drm picture provider to explain how all this works. Now, this should be handled with the latest rebase I guess. GBMPixmap will close the fd during destruction.
https://chromiumcodereview.appspot.com/490233002/diff/360001/ui/ozone/platfor... File ui/ozone/platform/dri/ozone_platform_gbm.cc (right): https://chromiumcodereview.appspot.com/490233002/diff/360001/ui/ozone/platfor... ui/ozone/platform/dri/ozone_platform_gbm.cc:118: if (!surface_factory_ozone_) Why is this needed ?
For the future, please try not to rebase before the review is over. It makes it very difficult to track changes between each patchset. If rebase is necessary during the review, please do it after making changes to address comments, not before, I can then at least see what changed before the rebase, instead of having to filter out rebase changes interleaved with actual CL changes. Thanks!
Stopped at providers with a realization. Please see comments for details. Thanks. https://chromiumcodereview.appspot.com/490233002/diff/360001/content/common/g... File content/common/gpu/media/rendering_helper.cc (right): https://chromiumcodereview.appspot.com/490233002/diff/360001/content/common/g... content/common/gpu/media/rendering_helper.cc:22: #include "ui/gl/gl_surface_egl.h" Should this be under GL_VARIANT_EGL below? https://chromiumcodereview.appspot.com/490233002/diff/360001/content/common/g... content/common/gpu/media/rendering_helper.cc:67: const int kTestWindowWidth = 800; I realized, could we use a fullscreen window? https://chromiumcodereview.appspot.com/490233002/diff/360001/content/common/g... content/common/gpu/media/rendering_helper.cc:88: ui::PlatformWindowState new_state) override{}; Some methods have spaces after override and before {}, some don't. https://chromiumcodereview.appspot.com/490233002/diff/360001/content/common/g... content/common/gpu/media/rendering_helper.cc:242: NULL, gl_surface_.get(), gfx::PreferIntegratedGpu); ozone_demo also calls gl_surface_->Resize() here, do we need to do the same? https://chromiumcodereview.appspot.com/490233002/diff/360001/content/common/g... content/common/gpu/media/rendering_helper.cc:243: gl_context_->MakeCurrent(gl_surface_.get()); Should we verify this succeeded? https://chromiumcodereview.appspot.com/490233002/diff/360001/content/common/g... content/common/gpu/media/rendering_helper.cc:369: WarmUpRendering(params.warm_up_iterations); Why is that if not needed anymore? https://chromiumcodereview.appspot.com/490233002/diff/360001/content/common/g... content/common/gpu/media/rendering_helper.cc:374: else You could just drop else. https://chromiumcodereview.appspot.com/490233002/diff/360001/content/common/g... content/common/gpu/media/rendering_helper.cc:745: // quick as possible. What if we are falling behind even if we are rendering as fast as possible? I think it would be better to make target = now and still go through the frame dropping mechanism. https://chromiumcodereview.appspot.com/490233002/diff/360001/content/common/g... File content/common/gpu/media/vaapi_h264_decoder_unittest.cc (right): https://chromiumcodereview.appspot.com/490233002/diff/360001/content/common/g... content/common/gpu/media/vaapi_h264_decoder_unittest.cc:90: // These members (x_display_, num_outputted_pictures_, num_surfaces_) No x_display_ anymore. https://chromiumcodereview.appspot.com/490233002/diff/360001/content/common/g... content/common/gpu/media/vaapi_h264_decoder_unittest.cc:105: // (2) The wrapper has a reference to x_display_. No x_display_ anymore. https://chromiumcodereview.appspot.com/490233002/diff/360001/content/common/g... content/common/gpu/media/vaapi_h264_decoder_unittest.cc:107: wrapper_ = NULL; I don't think we actually need these now that you don't have XCloseDisplay and the order of definition is correct (please add a comment). https://chromiumcodereview.appspot.com/490233002/diff/360001/content/common/g... File content/common/gpu/media/vaapi_picture_provider.h (right): https://chromiumcodereview.appspot.com/490233002/diff/360001/content/common/g... content/common/gpu/media/vaapi_picture_provider.h:28: class VaapiPictureProvider : public base::NonThreadSafe { Both VaapiWrapper and MakeContextCurrent are now owned by Picture instances. The only remaining thing we still need providers for is fb_config to be initialized somewhere. But if we perhaps used gfx::GLImageGLX in TFPPicture, then we wouldn't need that. And we could get rid of all the binding and glx code from TFPPicture. So maybe we could get rid of Providers entirely with that? https://chromiumcodereview.appspot.com/490233002/diff/360001/content/common/g... content/common/gpu/media/vaapi_picture_provider.h:30: // Picture is native pixmap abstraction (X11/Ozone) Please full stop at the end of sentences. https://chromiumcodereview.appspot.com/490233002/diff/360001/content/common/g... content/common/gpu/media/vaapi_picture_provider.h:44: virtual bool DownloadFromSurface(VASurfaceID va_surface_id, Could we use VASurface? We wounldn't need size then and we'd get refcounting.
posciak@chromium.org changed reviewers: + owenlin@chromium.org
Owen: could you please review changes to vdatest and RenderingHelper? Thanks.
https://codereview.chromium.org/490233002/diff/360001/content/common/gpu/medi... File content/common/gpu/media/rendering_helper.cc (right): https://codereview.chromium.org/490233002/diff/360001/content/common/gpu/medi... content/common/gpu/media/rendering_helper.cc:67: const int kTestWindowWidth = 800; On 2014/11/03 01:36:51, Pawel Osciak wrote: > I realized, could we use a fullscreen window? The performance on a non-fullscreen window is not optimized on some platforms (arm based). This is why we use a fullscreen window. https://codereview.chromium.org/490233002/diff/360001/content/common/gpu/medi... content/common/gpu/media/rendering_helper.cc:232: window_ = platform_window_delegate_->GetAcceleratedWidget(); How could you make sure the widget_ is available now ? https://codereview.chromium.org/490233002/diff/360001/content/common/gpu/medi... content/common/gpu/media/rendering_helper.cc:372: if (params.suppress_rendering) We use "frame_duration == base::TimeDelta()" to detect if the rendering is disabled. if (frame_duration_ != base::TimeDelta()) { WarmUpRendering(...); gl_surface_->GetVSyncProvider()->GetVSyncParameters(...); } else { done->Signal(); } I think the code depends on the availability of the VSync info. So, why do you have the case: "if we couldn't figure out the vsync_interval ...... ". https://codereview.chromium.org/490233002/diff/360001/content/common/gpu/medi... content/common/gpu/media/rendering_helper.cc:633: glClearColor(0.0f, 0.0f, 0.0f, 1.0f); Why we need to clear the color buffer, (I think video frames are opaque)? This will significantly affect the performance. https://codereview.chromium.org/490233002/diff/360001/content/common/gpu/medi... content/common/gpu/media/rendering_helper.cc:745: // quick as possible. On 2014/11/03 01:36:51, Pawel Osciak wrote: > What if we are falling behind even if we are rendering as fast as possible? I > think it would be better to make target = now and still go through the frame > dropping mechanism. Are you saying there won't be vsync info on ozone platform? We use vsync to prevent queuing swap buffers too fast. All the swap buffer blocked and affect decoding on VAAPI platforms. BTW, if there is no vsync info available, you should render at the |scheduled_render_time_| to keep the specified FPS. https://codereview.chromium.org/490233002/diff/360001/content/common/gpu/medi... File content/common/gpu/media/rendering_helper.h (right): https://codereview.chromium.org/490233002/diff/360001/content/common/gpu/medi... content/common/gpu/media/rendering_helper.h:67: bool suppress_rendering; Remove this ? Rebase conflict ? https://codereview.chromium.org/490233002/diff/360001/content/common/gpu/medi... File content/common/gpu/media/video_decode_accelerator_unittest.cc (right): https://codereview.chromium.org/490233002/diff/360001/content/common/gpu/medi... content/common/gpu/media/video_decode_accelerator_unittest.cc:37: #include "base/message_loop/message_loop.h" Why these header files are included now ? https://codereview.chromium.org/490233002/diff/360001/content/common/gpu/medi... content/common/gpu/media/video_decode_accelerator_unittest.cc:105: // least one frame for backends like Ozone, because as opposed to the I think we should move the code into rendering helper. The value will be override by command line switch. If we must warm up for at least one frame, we should move the comment there and with something like: #ifdef(USE_OZONE) warmup_iteration = std::max(1, warmup_iteration); #endif https://codereview.chromium.org/490233002/diff/360001/content/common/gpu/medi... content/common/gpu/media/video_decode_accelerator_unittest.cc:1354: helper_params.suppress_rendering = true; rendering_fps == 0 indicates supporess_rendering.
https://codereview.chromium.org/490233002/diff/360001/content/common/gpu/medi... File content/common/gpu/media/rendering_helper.cc (right): https://codereview.chromium.org/490233002/diff/360001/content/common/gpu/medi... content/common/gpu/media/rendering_helper.cc:369: WarmUpRendering(params.warm_up_iterations); On 2014/11/03 01:36:51, Pawel Osciak wrote: > Why is that if not needed anymore? That seems to be a workaround for drivers that are slow to render the first few frames, I suppose Owen would know more. https://codereview.chromium.org/490233002/diff/360001/content/common/gpu/medi... content/common/gpu/media/rendering_helper.cc:372: if (params.suppress_rendering) On 2014/11/03 06:59:06, Owen Lin wrote: > We use "frame_duration == base::TimeDelta()" to detect if the rendering is > disabled. > > if (frame_duration_ != base::TimeDelta()) { > WarmUpRendering(...); > gl_surface_->GetVSyncProvider()->GetVSyncParameters(...); > } else { > done->Signal(); > } > > I think the code depends on the availability of the VSync info. So, why do you > have the case: "if we couldn't figure out the vsync_interval ...... ". To get a vsync_internal value, we need to render at least once. If there is no rendering, we wait forever a vsync even that will never happen. My understanding is that if you have a gpu process running, then you can get an answer without ever rendering anything because there has been frames rendered and displayed before, whereas the Ozone setup for this test doesn't use the gpu-process and we have no history to compute the vsync_interval. https://codereview.chromium.org/490233002/diff/360001/content/common/gpu/medi... content/common/gpu/media/rendering_helper.cc:374: else On 2014/11/03 01:36:51, Pawel Osciak wrote: > You could just drop else. Ok, I would need to remove the done pointer too. https://codereview.chromium.org/490233002/diff/360001/content/common/gpu/medi... content/common/gpu/media/rendering_helper.cc:633: glClearColor(0.0f, 0.0f, 0.0f, 1.0f); On 2014/11/03 06:59:06, Owen Lin wrote: > Why we need to clear the color buffer, (I think video frames are opaque)? This > will significantly affect the performance. This was to workaround a bug in Mesa : http://cgit.freedesktop.org/mesa/mesa/commit/src?id=d175e7c16b4de1a4231b806da... But I think you fixed this issue with need_swap_buffer. https://codereview.chromium.org/490233002/diff/360001/content/common/gpu/medi... content/common/gpu/media/rendering_helper.cc:745: // quick as possible. On 2014/11/03 06:59:05, Owen Lin wrote: > On 2014/11/03 01:36:51, Pawel Osciak wrote: > > What if we are falling behind even if we are rendering as fast as possible? I > > think it would be better to make target = now and still go through the frame > > dropping mechanism. > > Are you saying there won't be vsync info on ozone platform? We use vsync to > prevent queuing swap buffers too fast. All the swap buffer blocked and affect > decoding on VAAPI platforms. > > BTW, if there is no vsync info available, you should render at the > |scheduled_render_time_| to keep the specified FPS. As explained in another comment, we won't get a vsync_interval on Ozone until we render+swapBuffer a first frame. So this is just for a case for the first frame. https://codereview.chromium.org/490233002/diff/360001/content/common/gpu/medi... File content/common/gpu/media/rendering_helper.h (right): https://codereview.chromium.org/490233002/diff/360001/content/common/gpu/medi... content/common/gpu/media/rendering_helper.h:67: bool suppress_rendering; On 2014/11/03 06:59:06, Owen Lin wrote: > Remove this ? Rebase conflict ? As you suggested in a different comment, I will use the frame_duration_ instead of this flag. https://codereview.chromium.org/490233002/diff/360001/content/common/gpu/medi... File content/common/gpu/media/vaapi_h264_decoder_unittest.cc (right): https://codereview.chromium.org/490233002/diff/360001/content/common/gpu/medi... content/common/gpu/media/vaapi_h264_decoder_unittest.cc:90: // These members (x_display_, num_outputted_pictures_, num_surfaces_) On 2014/11/03 01:36:51, Pawel Osciak wrote: > No x_display_ anymore. Acknowledged. https://codereview.chromium.org/490233002/diff/360001/content/common/gpu/medi... content/common/gpu/media/vaapi_h264_decoder_unittest.cc:105: // (2) The wrapper has a reference to x_display_. On 2014/11/03 01:36:51, Pawel Osciak wrote: > No x_display_ anymore. Acknowledged. https://codereview.chromium.org/490233002/diff/360001/content/common/gpu/medi... content/common/gpu/media/vaapi_h264_decoder_unittest.cc:107: wrapper_ = NULL; On 2014/11/03 01:36:51, Pawel Osciak wrote: > I don't think we actually need these now that you don't have XCloseDisplay and > the order of definition is correct (please add a comment). Acknowledged. https://codereview.chromium.org/490233002/diff/360001/content/common/gpu/medi... File content/common/gpu/media/vaapi_picture_provider.h (right): https://codereview.chromium.org/490233002/diff/360001/content/common/gpu/medi... content/common/gpu/media/vaapi_picture_provider.h:28: class VaapiPictureProvider : public base::NonThreadSafe { On 2014/11/03 01:36:51, Pawel Osciak wrote: > Both VaapiWrapper and MakeContextCurrent are now owned by Picture instances. The > only remaining thing we still need providers for is fb_config to be initialized > somewhere. But if we perhaps used gfx::GLImageGLX in TFPPicture, then we > wouldn't need that. And we could get rid of all the binding and glx code from > TFPPicture. > So maybe we could get rid of Providers entirely with that? The creation of pictures is fairly different between X11 and DRM. The way we "blit" from NV12 to RGBA is different too. So we need at least a class to abstract these differences. Would you like to go back to the first iteration of this CL, where all of the picture creation was inside VaapiWrapper? https://codereview.chromium.org/490233002/diff/360001/content/common/gpu/medi... content/common/gpu/media/vaapi_picture_provider.h:30: // Picture is native pixmap abstraction (X11/Ozone) On 2014/11/03 01:36:51, Pawel Osciak wrote: > Please full stop at the end of sentences. Acknowledged. https://codereview.chromium.org/490233002/diff/360001/content/common/gpu/medi... content/common/gpu/media/vaapi_picture_provider.h:44: virtual bool DownloadFromSurface(VASurfaceID va_surface_id, On 2014/11/03 01:36:51, Pawel Osciak wrote: > Could we use VASurface? We wounldn't need size then and we'd get refcounting. Acknowledged. https://codereview.chromium.org/490233002/diff/360001/content/common/gpu/medi... File content/common/gpu/media/video_decode_accelerator_unittest.cc (right): https://codereview.chromium.org/490233002/diff/360001/content/common/gpu/medi... content/common/gpu/media/video_decode_accelerator_unittest.cc:105: // least one frame for backends like Ozone, because as opposed to the On 2014/11/03 06:59:06, Owen Lin wrote: > I think we should move the code into rendering helper. > > The value will be override by command line switch. If we must warm up for at > least one frame, we should move the comment there and with something like: > > #ifdef(USE_OZONE) > warmup_iteration = std::max(1, warmup_iteration); > #endif Acknowledged. https://codereview.chromium.org/490233002/diff/360001/content/common/gpu/medi... content/common/gpu/media/video_decode_accelerator_unittest.cc:1354: helper_params.suppress_rendering = true; On 2014/11/03 06:59:06, Owen Lin wrote: > rendering_fps == 0 indicates supporess_rendering. Acknowledged. https://codereview.chromium.org/490233002/diff/360001/content/content_common.... File content/content_common.gypi (right): https://codereview.chromium.org/490233002/diff/360001/content/content_common.... content/content_common.gypi:775: ['target_arch != "arm" and chromeos == 1', { On 2014/10/31 19:07:58, vignatti wrote: > here we need to opt out also platforms that don't provide hw acceleration, e.g. > Ozone DRI, testing etc Acknowledged. https://codereview.chromium.org/490233002/diff/360001/ui/ozone/platform/dri/o... File ui/ozone/platform/dri/ozone_platform_gbm.cc (right): https://codereview.chromium.org/490233002/diff/360001/ui/ozone/platform/dri/o... ui/ozone/platform/dri/ozone_platform_gbm.cc:118: if (!surface_factory_ozone_) On 2014/11/01 19:28:35, kalyank wrote: > Why is this needed ? We don't want to initialize the surface factory twice (when calling InitializeUI and InitializeGPU).
https://codereview.chromium.org/490233002/diff/360001/ui/ozone/platform/dri/o... File ui/ozone/platform/dri/ozone_platform_gbm.cc (right): https://codereview.chromium.org/490233002/diff/360001/ui/ozone/platform/dri/o... ui/ozone/platform/dri/ozone_platform_gbm.cc:118: if (!surface_factory_ozone_) On 2014/11/03 09:30:11, llandwerlin wrote: > On 2014/11/01 19:28:35, kalyank wrote: > > Why is this needed ? > > We don't want to initialize the surface factory twice (when calling InitializeUI > and InitializeGPU). This should be the case only when in single process mode, other wise we should be initializing them in different process (InitializeUI should be called from Browser process and GPU from GPU process respectively.) Even in single process case, UI should be called first and then GPU. I think we should avoid this change as part of this CL and move it to a separate CL. spang ^
https://codereview.chromium.org/490233002/diff/360001/ui/ozone/platform/dri/o... File ui/ozone/platform/dri/ozone_platform_gbm.cc (right): https://codereview.chromium.org/490233002/diff/360001/ui/ozone/platform/dri/o... ui/ozone/platform/dri/ozone_platform_gbm.cc:118: if (!surface_factory_ozone_) On 2014/11/04 01:04:19, kalyank wrote: > On 2014/11/03 09:30:11, llandwerlin wrote: > > On 2014/11/01 19:28:35, kalyank wrote: > > > Why is this needed ? > > > > We don't want to initialize the surface factory twice (when calling > InitializeUI > > and InitializeGPU). > > This should be the case only when in single process mode, other wise we should > be initializing them in different process (InitializeUI should be called from > Browser process and GPU from GPU process respectively.) Even in single process > case, UI should be called first and then GPU. I think we should avoid this > change as part of this CL and move it to a separate CL. > spang ^ > When running in-process, InitializeUI() should be called first, that's why the if-statement is present in InitializeGPU().
https://codereview.chromium.org/490233002/diff/360001/content/common/gpu/medi... File content/common/gpu/media/rendering_helper.cc (right): https://codereview.chromium.org/490233002/diff/360001/content/common/gpu/medi... content/common/gpu/media/rendering_helper.cc:369: WarmUpRendering(params.warm_up_iterations); On 2014/11/03 09:30:10, llandwerlin wrote: > On 2014/11/03 01:36:51, Pawel Osciak wrote: > > Why is that if not needed anymore? > > That seems to be a workaround for drivers that are slow to render the first few > frames, I suppose Owen would know more. I think you still need the if. We use frame_duration == 0 as a hint that the rendering is disabled. https://codereview.chromium.org/490233002/diff/360001/content/common/gpu/medi... content/common/gpu/media/rendering_helper.cc:372: if (params.suppress_rendering) On 2014/11/03 09:30:10, llandwerlin wrote: > On 2014/11/03 06:59:06, Owen Lin wrote: > > We use "frame_duration == base::TimeDelta()" to detect if the rendering is > > disabled. > > > > if (frame_duration_ != base::TimeDelta()) { > > WarmUpRendering(...); > > gl_surface_->GetVSyncProvider()->GetVSyncParameters(...); > > } else { > > done->Signal(); > > } > > > > I think the code depends on the availability of the VSync info. So, why do you > > have the case: "if we couldn't figure out the vsync_interval ...... ". > > To get a vsync_internal value, we need to render at least once. If there is no > rendering, we wait forever a vsync even that will never happen. > My understanding is that if you have a gpu process running, then you can get an > answer without ever rendering anything because there has been frames rendered > and displayed before, whereas the Ozone setup for this test doesn't use the > gpu-process and we have no history to compute the vsync_interval. But it is not what I saw. In vda_unittest, there is no gpu process, but I always got the vsync info sucessfully without rendering anything. Theoretically, VSync should be triggered even without rendering. If this is the case for ozone, maybe we need the WarmUpRendering to provide us the vsync information. In that case, please make sure WarmUpRendering is called with warm_up_iterations >= 1 and add comments to explain the details. https://codereview.chromium.org/490233002/diff/360001/content/common/gpu/medi... content/common/gpu/media/rendering_helper.cc:745: // quick as possible. On 2014/11/03 09:30:10, llandwerlin wrote: > On 2014/11/03 06:59:05, Owen Lin wrote: > > On 2014/11/03 01:36:51, Pawel Osciak wrote: > > > What if we are falling behind even if we are rendering as fast as possible? > I > > > think it would be better to make target = now and still go through the frame > > > dropping mechanism. > > > > Are you saying there won't be vsync info on ozone platform? We use vsync to > > prevent queuing swap buffers too fast. All the swap buffer blocked and affect > > decoding on VAAPI platforms. > > > > BTW, if there is no vsync info available, you should render at the > > |scheduled_render_time_| to keep the specified FPS. > > As explained in another comment, we won't get a vsync_interval on Ozone until we > render+swapBuffer a first frame. So this is just for a case for the first frame. We will wait forever on getting the vsync info in Initialize(). So, it would be a deadlock if there is no vsync provided. We don't need to handle the case here.
lionel.g.landwerlin@intel.com changed reviewers: + reveman@chromium.org
reveman@chromium.org: Please review changes in ui/gl/gl_image_glx.cc, Thank you. https://codereview.chromium.org/490233002/diff/360001/content/common/gpu/medi... File content/common/gpu/media/rendering_helper.cc (right): https://codereview.chromium.org/490233002/diff/360001/content/common/gpu/medi... content/common/gpu/media/rendering_helper.cc:88: ui::PlatformWindowState new_state) override{}; On 2014/11/03 01:36:51, Pawel Osciak wrote: > Some methods have spaces after override and before {}, some don't. Acknowledged. https://codereview.chromium.org/490233002/diff/360001/content/common/gpu/medi... content/common/gpu/media/rendering_helper.cc:232: window_ = platform_window_delegate_->GetAcceleratedWidget(); On 2014/11/03 06:59:05, Owen Lin wrote: > How could you make sure the widget_ is available now ? When the DriWindow is created and initialized, it immediately calls OnAcceleratedWidgetAvailable() on the window delegate with the widget id. If that's not the case, it means we failed to create a window and the test will fail on the : CHECK(window_ != gfx::kNullAcceleratedWidget); https://codereview.chromium.org/490233002/diff/360001/content/common/gpu/medi... content/common/gpu/media/rendering_helper.cc:242: NULL, gl_surface_.get(), gfx::PreferIntegratedGpu); On 2014/11/03 01:36:51, Pawel Osciak wrote: > ozone_demo also calls gl_surface_->Resize() here, do we need to do the same? I haven't identified an ozone api to return the size of the screen. But surfaces seems to be created at the size of the screen so I'll use that instead : https://code.google.com/p/chromium/codesearch#chromium/src/ui/ozone/platform/... https://codereview.chromium.org/490233002/diff/360001/content/common/gpu/medi... content/common/gpu/media/rendering_helper.cc:243: gl_context_->MakeCurrent(gl_surface_.get()); On 2014/11/03 01:36:50, Pawel Osciak wrote: > Should we verify this succeeded? Acknowledged. https://codereview.chromium.org/490233002/diff/360001/content/common/gpu/medi... content/common/gpu/media/rendering_helper.cc:633: glClearColor(0.0f, 0.0f, 0.0f, 1.0f); On 2014/11/03 09:30:10, llandwerlin wrote: > On 2014/11/03 06:59:06, Owen Lin wrote: > > Why we need to clear the color buffer, (I think video frames are opaque)? This > > will significantly affect the performance. > > This was to workaround a bug in Mesa : > > http://cgit.freedesktop.org/mesa/mesa/commit/src?id=d175e7c16b4de1a4231b806da... > > But I think you fixed this issue with need_swap_buffer. I need to leave it for the non thumbnail case, otherwise you can see previous tests' last frame. https://codereview.chromium.org/490233002/diff/360001/content/common/gpu/medi... File content/common/gpu/media/video_decode_accelerator_unittest.cc (right): https://codereview.chromium.org/490233002/diff/360001/content/common/gpu/medi... content/common/gpu/media/video_decode_accelerator_unittest.cc:37: #include "base/message_loop/message_loop.h" On 2014/11/03 06:59:06, Owen Lin wrote: > Why these header files are included now ? Removing.
https://codereview.chromium.org/490233002/diff/360001/ui/ozone/platform/dri/o... File ui/ozone/platform/dri/ozone_platform_gbm.cc (right): https://codereview.chromium.org/490233002/diff/360001/ui/ozone/platform/dri/o... ui/ozone/platform/dri/ozone_platform_gbm.cc:118: if (!surface_factory_ozone_) > if-statement is present in InitializeGPU(). Ya, so we don't need this change basically.
PTAL, Thanks.
https://codereview.chromium.org/490233002/diff/420001/content/common/gpu/medi... File content/common/gpu/media/rendering_helper.cc (right): https://codereview.chromium.org/490233002/diff/420001/content/common/gpu/medi... content/common/gpu/media/rendering_helper.cc:644: glClear(GL_COLOR_BUFFER_BIT); I'm not sure why this is needed now? https://codereview.chromium.org/490233002/diff/420001/ui/gl/gl_image_glx.cc File ui/gl/gl_image_glx.cc (right): https://codereview.chromium.org/490233002/diff/420001/ui/gl/gl_image_glx.cc#n... ui/gl/gl_image_glx.cc:41: return GLX_BIND_TO_TEXTURE_RGB_EXT; GLX_TEXTURE_FORMAT_RGB_EXT
https://codereview.chromium.org/490233002/diff/420001/ui/gl/gl_image_glx.cc File ui/gl/gl_image_glx.cc (right): https://codereview.chromium.org/490233002/diff/420001/ui/gl/gl_image_glx.cc#n... ui/gl/gl_image_glx.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. This filed doesn't exist in ToT. Why did you have to make changes to it?
On 2014/11/04 21:14:44, reveman wrote: > https://codereview.chromium.org/490233002/diff/420001/ui/gl/gl_image_glx.cc > File ui/gl/gl_image_glx.cc (right): > > https://codereview.chromium.org/490233002/diff/420001/ui/gl/gl_image_glx.cc#n... > ui/gl/gl_image_glx.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights > reserved. > This filed doesn't exist in ToT. Why did you have to make changes to it? @reveman Pawel asked to use GLImageGLX, to remove some code from the Vaapi VDA. Do you think it make sense to revive it if there is a potential user (this CL : https://codereview.chromium.org/490233002/patch/420001/430016)?
https://codereview.chromium.org/490233002/diff/420001/content/common/gpu/medi... File content/common/gpu/media/rendering_helper.cc (right): https://codereview.chromium.org/490233002/diff/420001/content/common/gpu/medi... content/common/gpu/media/rendering_helper.cc:644: glClear(GL_COLOR_BUFFER_BIT); On 2014/11/04 19:25:38, marcheu wrote: > I'm not sure why this is needed now? Yes, otherwise I can see the last frame of the previous test every other frame. https://codereview.chromium.org/490233002/diff/420001/ui/gl/gl_image_glx.cc File ui/gl/gl_image_glx.cc (right): https://codereview.chromium.org/490233002/diff/420001/ui/gl/gl_image_glx.cc#n... ui/gl/gl_image_glx.cc:41: return GLX_BIND_TO_TEXTURE_RGB_EXT; On 2014/11/04 19:25:38, marcheu wrote: > GLX_TEXTURE_FORMAT_RGB_EXT Thanks, fixed.
On 2014/11/05 10:04:33, llandwerlin wrote: > On 2014/11/04 21:14:44, reveman wrote: > > https://codereview.chromium.org/490233002/diff/420001/ui/gl/gl_image_glx.cc > > File ui/gl/gl_image_glx.cc (right): > > > > > https://codereview.chromium.org/490233002/diff/420001/ui/gl/gl_image_glx.cc#n... > > ui/gl/gl_image_glx.cc:1: // Copyright (c) 2012 The Chromium Authors. All > rights > > reserved. > > This filed doesn't exist in ToT. Why did you have to make changes to it? > > @reveman Pawel asked to use GLImageGLX, to remove some code from the Vaapi VDA. > Do you think it make sense to revive it if there is a potential user (this CL : > https://codereview.chromium.org/490233002/patch/420001/430016) Just adding back ui/gl/gl_image_glx.* would be fine if that makes sense. I'd rather not reintroduce the X11_PIMAP gpu memory buffer type but I assume you don't actually need that in this case. What code is being removed by using this?
On 2014/11/05 16:28:41, reveman wrote: > On 2014/11/05 10:04:33, llandwerlin wrote: > > On 2014/11/04 21:14:44, reveman wrote: > > > https://codereview.chromium.org/490233002/diff/420001/ui/gl/gl_image_glx.cc > > > File ui/gl/gl_image_glx.cc (right): > > > > > > > > > https://codereview.chromium.org/490233002/diff/420001/ui/gl/gl_image_glx.cc#n... > > > ui/gl/gl_image_glx.cc:1: // Copyright (c) 2012 The Chromium Authors. All > > rights > > > reserved. > > > This filed doesn't exist in ToT. Why did you have to make changes to it? > > > > @reveman Pawel asked to use GLImageGLX, to remove some code from the Vaapi > VDA. > > Do you think it make sense to revive it if there is a potential user (this CL > : > > https://codereview.chromium.org/490233002/patch/420001/430016) > > Just adding back ui/gl/gl_image_glx.* would be fine if that makes sense. I'd > rather not reintroduce the X11_PIMAP gpu memory buffer type but I assume you > don't actually need that in this case. > > What code is being removed by using this? These 2 functions currently in the VDA : VaapiVideoDecodeAccelerator::InitializeFBConfig() : https://code.google.com/p/chromium/codesearch#chromium/src/content/common/gpu... VaapiVideoDecodeAccelerator::TFPPicture::Initialize(): https://code.google.com/p/chromium/codesearch#chromium/src/content/common/gpu...
On 2014/11/05 16:36:05, llandwerlin wrote: > On 2014/11/05 16:28:41, reveman wrote: > > On 2014/11/05 10:04:33, llandwerlin wrote: > > > On 2014/11/04 21:14:44, reveman wrote: > > > > > https://codereview.chromium.org/490233002/diff/420001/ui/gl/gl_image_glx.cc > > > > File ui/gl/gl_image_glx.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/490233002/diff/420001/ui/gl/gl_image_glx.cc#n... > > > > ui/gl/gl_image_glx.cc:1: // Copyright (c) 2012 The Chromium Authors. All > > > rights > > > > reserved. > > > > This filed doesn't exist in ToT. Why did you have to make changes to it? > > > > > > @reveman Pawel asked to use GLImageGLX, to remove some code from the Vaapi > > VDA. > > > Do you think it make sense to revive it if there is a potential user (this > CL > > : > > > https://codereview.chromium.org/490233002/patch/420001/430016) > > > > Just adding back ui/gl/gl_image_glx.* would be fine if that makes sense. I'd > > rather not reintroduce the X11_PIMAP gpu memory buffer type but I assume you > > don't actually need that in this case. > > > > What code is being removed by using this? > > These 2 functions currently in the VDA : > > VaapiVideoDecodeAccelerator::InitializeFBConfig() : > https://code.google.com/p/chromium/codesearch#chromium/src/content/common/gpu... > > VaapiVideoDecodeAccelerator::TFPPicture::Initialize(): > https://code.google.com/p/chromium/codesearch#chromium/src/content/common/gpu... Ok, moving the code behind the GLImage API sgtm. You can just add back the last version of ui/gl/gl_image_glx.* files with your modifications as part of this patch if you like.
alexst@chromium.org changed reviewers: + alexst@chromium.org
https://codereview.chromium.org/490233002/diff/440001/content/common/gpu/medi... File content/common/gpu/media/vaapi_drm_picture.cc (right): https://codereview.chromium.org/490233002/diff/440001/content/common/gpu/medi... content/common/gpu/media/vaapi_drm_picture.cc:111: EGL_NATIVE_PIXMAP_KHR, pixmap_->GetEGLClientBuffer(), attrs)) { Driveby: pixmap_ will have either a dmabuf or a client buffer, not both. Right now the gbm platform is returning a dmabuf as used above. You can use gfx::GLImageLinuxDMABuffer here instead of gfx::GLImageEGL to make it work.
> Driveby: pixmap_ will have either a dmabuf or a client buffer, not both. Right > now the gbm platform is returning a dmabuf as used above. > > You can use gfx::GLImageLinuxDMABuffer here instead of gfx::GLImageEGL to make > it work. Hit send too early. Adding both, client buffer and dmabuf to GbmPixmap would break surfaceless rendering, which we are about to turn on by default. We left client bitmap there for now in case rendering with dmabuf had issues. Anyway, using gfx::GLImageLinuxDMABuffer here would remove the need for changes in gbm_buffer.cc.
https://chromiumcodereview.appspot.com/490233002/diff/420001/content/common/g... File content/common/gpu/media/rendering_helper.cc (right): https://chromiumcodereview.appspot.com/490233002/diff/420001/content/common/g... content/common/gpu/media/rendering_helper.cc:71: const int kTestWindowHeight = 1; How about const gfx::Rect kTestWindowSize(1, 1); ? Why not just use an empty Rect ? https://chromiumcodereview.appspot.com/490233002/diff/420001/content/common/g... content/common/gpu/media/rendering_helper.cc:197: screen_size_.width(), Remove line 189 (screen_size_ = ...) and just use GetSystemMetrics(MS_CXSCREEN) here. You will update screen_size_ later. Same as the case for USE_X11. https://chromiumcodereview.appspot.com/490233002/diff/420001/content/common/g... content/common/gpu/media/rendering_helper.cc:386: gl_surface_->GetVSyncProvider()->GetVSyncParameters(base::Bind( I like your idea that don't call this function if rendering is disabled. When rendering is disabled, this class is only used to provide textures for the video decoder. All the rendering related functions won't be called. https://chromiumcodereview.appspot.com/490233002/diff/420001/content/common/g... content/common/gpu/media/rendering_helper.cc:767: // If we don't have a frame_duration_, just drop as many frames as This is not needed. If frame_duration is null, we won't call RenderContent() (and related functions) at all.
On 2014/11/05 10:17:35, llandwerlin wrote: > https://codereview.chromium.org/490233002/diff/420001/content/common/gpu/medi... > File content/common/gpu/media/rendering_helper.cc (right): > > https://codereview.chromium.org/490233002/diff/420001/content/common/gpu/medi... > content/common/gpu/media/rendering_helper.cc:644: glClear(GL_COLOR_BUFFER_BIT); > On 2014/11/04 19:25:38, marcheu wrote: > > I'm not sure why this is needed now? > > Yes, otherwise I can see the last frame of the previous test every other frame. > But what is the reason for that, and why is it different from before? > https://codereview.chromium.org/490233002/diff/420001/ui/gl/gl_image_glx.cc > File ui/gl/gl_image_glx.cc (right): > > https://codereview.chromium.org/490233002/diff/420001/ui/gl/gl_image_glx.cc#n... > ui/gl/gl_image_glx.cc:41: return GLX_BIND_TO_TEXTURE_RGB_EXT; > On 2014/11/04 19:25:38, marcheu wrote: > > GLX_TEXTURE_FORMAT_RGB_EXT > > Thanks, fixed.
On 2014/11/06 19:54:59, marcheu1 wrote: > On 2014/11/05 10:17:35, llandwerlin wrote: > > > https://codereview.chromium.org/490233002/diff/420001/content/common/gpu/medi... > > File content/common/gpu/media/rendering_helper.cc (right): > > > > > https://codereview.chromium.org/490233002/diff/420001/content/common/gpu/medi... > > content/common/gpu/media/rendering_helper.cc:644: > glClear(GL_COLOR_BUFFER_BIT); > > On 2014/11/04 19:25:38, marcheu wrote: > > > I'm not sure why this is needed now? > > > > Yes, otherwise I can see the last frame of the previous test every other > frame. > > > > But what is the reason for that, and why is it different from before? > I'm not sure we have a reference (in the chromium tests) for what happens when a GL application renders into a buffers without ever clearing it on Ozone, but it seems logical to render on top of what was previously there. On X, I suppose the CWBackPixel attribute passed to XCreateWindow will ensure the window is initialized with a backing buffer set to black. I can get rid of the glClear() in RenderContent() if I make sure to clear all the buffers. WarmUpRendering() does something very similar to a glClear(), so calling WarmUpRendering() with a parameter 2 seems to be enough clear the double buffered surface.
https://codereview.chromium.org/490233002/diff/420001/content/common/gpu/medi... File content/common/gpu/media/rendering_helper.cc (right): https://codereview.chromium.org/490233002/diff/420001/content/common/gpu/medi... content/common/gpu/media/rendering_helper.cc:71: const int kTestWindowHeight = 1; On 2014/11/06 05:45:01, Owen Lin wrote: > How about const gfx::Rect kTestWindowSize(1, 1); ? > Why not just use an empty Rect ? Added an empty Rect in the StubOzoneDelegate constructor. https://codereview.chromium.org/490233002/diff/420001/content/common/gpu/medi... content/common/gpu/media/rendering_helper.cc:197: screen_size_.width(), On 2014/11/06 05:45:01, Owen Lin wrote: > Remove line 189 (screen_size_ = ...) and just use GetSystemMetrics(MS_CXSCREEN) > here. > > You will update screen_size_ later. > > Same as the case for USE_X11. Done. https://codereview.chromium.org/490233002/diff/420001/content/common/gpu/medi... content/common/gpu/media/rendering_helper.cc:767: // If we don't have a frame_duration_, just drop as many frames as On 2014/11/06 05:45:01, Owen Lin wrote: > This is not needed. If frame_duration is null, we won't call RenderContent() > (and related functions) at all. Done. https://codereview.chromium.org/490233002/diff/440001/content/common/gpu/medi... File content/common/gpu/media/vaapi_drm_picture.cc (right): https://codereview.chromium.org/490233002/diff/440001/content/common/gpu/medi... content/common/gpu/media/vaapi_drm_picture.cc:111: EGL_NATIVE_PIXMAP_KHR, pixmap_->GetEGLClientBuffer(), attrs)) { On 2014/11/05 20:58:25, alexst wrote: > Driveby: pixmap_ will have either a dmabuf or a client buffer, not both. Right > now the gbm platform is returning a dmabuf as used above. > > You can use gfx::GLImageLinuxDMABuffer here instead of gfx::GLImageEGL to make > it work. Done.
PTAL, Thanks.
lgtm % nits https://codereview.chromium.org/490233002/diff/480001/content/common/gpu/medi... File content/common/gpu/media/rendering_helper.cc (right): https://codereview.chromium.org/490233002/diff/480001/content/common/gpu/medi... content/common/gpu/media/rendering_helper.cc:393: scoped_ptr<GLubyte[]> emptyData(new GLubyte[screen_size_.GetArea() * 2]); Should we initialize the buffer to make it black? new GLubyte[screen_size_.GetArea() * 2]()
https://codereview.chromium.org/490233002/diff/480001/content/common/gpu/medi... File content/common/gpu/media/rendering_helper.cc (right): https://codereview.chromium.org/490233002/diff/480001/content/common/gpu/medi... content/common/gpu/media/rendering_helper.cc:393: scoped_ptr<GLubyte[]> emptyData(new GLubyte[screen_size_.GetArea() * 2]); On 2014/11/11 03:52:10, Owen Lin wrote: > Should we initialize the buffer to make it black? > > new GLubyte[screen_size_.GetArea() * 2]() Done.
re: driveby in vaapi_drm_picture.cc lgtm
https://codereview.chromium.org/490233002/diff/480001/content/common/gpu/medi... File content/common/gpu/media/va_surface.h (right): https://codereview.chromium.org/490233002/diff/480001/content/common/gpu/medi... content/common/gpu/media/va_surface.h:68: // | available after the client of VVDA returns | Broken comment. https://codereview.chromium.org/490233002/diff/480001/content/common/gpu/medi... File content/common/gpu/media/vaapi_drm_picture.cc (right): https://codereview.chromium.org/490233002/diff/480001/content/common/gpu/medi... content/common/gpu/media/vaapi_drm_picture.cc:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. No "(c)" in new files please. Please use http://www.chromium.org/developers/coding-style#TOC-File-headers for the correct header. https://codereview.chromium.org/490233002/diff/480001/content/common/gpu/medi... content/common/gpu/media/vaapi_drm_picture.cc:64: va_attrib_extbuf.pixel_format = VA_FOURCC_BGRX; Perhaps memset it to 0 first and also drop 0-assignments? https://codereview.chromium.org/490233002/diff/480001/content/common/gpu/medi... content/common/gpu/media/vaapi_drm_picture.cc:86: va_surface_ = vaapi_wrapper_->CreateUnownedSurface( You can't dereference a WeakPtr like this. It will return NULL if it's no longer valid (please see explanation at https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/weak_p...). https://codereview.chromium.org/490233002/diff/480001/content/common/gpu/medi... content/common/gpu/media/vaapi_drm_picture.cc:102: } Do we need to make context current somewhere around here? https://codereview.chromium.org/490233002/diff/480001/content/common/gpu/medi... content/common/gpu/media/vaapi_drm_picture.cc:106: if (!egl_image_->BindTexImage(GL_TEXTURE_EXTERNAL_OES)) { Do we need to ReleaseTexImage() somewhere? https://codereview.chromium.org/490233002/diff/480001/content/common/gpu/medi... content/common/gpu/media/vaapi_drm_picture.cc:127: uint32 VaapiPicture::GetGLTextureTarget() { I feel this is not a good way to do this. Please make it non-static and implement in VaapiDrmPicture and TFP. https://codereview.chromium.org/490233002/diff/480001/content/common/gpu/medi... File content/common/gpu/media/vaapi_drm_picture.h (right): https://codereview.chromium.org/490233002/diff/480001/content/common/gpu/medi... content/common/gpu/media/vaapi_drm_picture.h:15: #include "third_party/libva/va/va.h" This include should probably be in the .cc file. https://codereview.chromium.org/490233002/diff/480001/content/common/gpu/medi... File content/common/gpu/media/vaapi_h264_decoder_unittest.cc (right): https://codereview.chromium.org/490233002/diff/480001/content/common/gpu/medi... content/common/gpu/media/vaapi_h264_decoder_unittest.cc:84: scoped_ptr<VaapiWrapper> wrapper_; Please add a comment about the order. https://codereview.chromium.org/490233002/diff/480001/content/common/gpu/medi... File content/common/gpu/media/vaapi_picture.h (right): https://codereview.chromium.org/490233002/diff/480001/content/common/gpu/medi... content/common/gpu/media/vaapi_picture.h:42: // Returns the type of GL texture used bind images (either Please rephrase... https://codereview.chromium.org/490233002/diff/480001/content/common/gpu/medi... File content/common/gpu/media/vaapi_tfp_picture.cc (right): https://codereview.chromium.org/490233002/diff/480001/content/common/gpu/medi... content/common/gpu/media/vaapi_tfp_picture.cc:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. no "(c)" please. https://codereview.chromium.org/490233002/diff/480001/content/common/gpu/medi... File content/common/gpu/media/vaapi_tfp_picture.h (right): https://codereview.chromium.org/490233002/diff/480001/content/common/gpu/medi... content/common/gpu/media/vaapi_tfp_picture.h:15: #include "third_party/libva/va/va.h" Not needed? https://codereview.chromium.org/490233002/diff/480001/content/common/gpu/medi... File content/common/gpu/media/vaapi_video_decode_accelerator.cc (right): https://codereview.chromium.org/490233002/diff/480001/content/common/gpu/medi... content/common/gpu/media/vaapi_video_decode_accelerator.cc:101: // These references are destroyed when we call vaTerminate(), this Could you identify the calls that destroy them? Could we move them to the end of vaPutSurface instead? Otherwise, I think then maybe we'd have to make VaapiWrapper the producer and owner of these until we fix this issue. On destruction, it would have to do the dance in the correct order. It's preferable not to concern VAVDA with this. But in any case, could you please point to the place in libva that gets and puts the reference? I'd prefer to move the put call to end of vaPutSurface, we could have a local patch in our repo for now.
https://codereview.chromium.org/490233002/diff/480001/content/common/gpu/medi... File content/common/gpu/media/va_surface.h (right): https://codereview.chromium.org/490233002/diff/480001/content/common/gpu/medi... content/common/gpu/media/va_surface.h:68: // | available after the client of VVDA returns | On 2014/11/12 05:52:57, Pawel Osciak wrote: > Broken comment. Acknowledged. https://codereview.chromium.org/490233002/diff/480001/content/common/gpu/medi... File content/common/gpu/media/vaapi_drm_picture.cc (right): https://codereview.chromium.org/490233002/diff/480001/content/common/gpu/medi... content/common/gpu/media/vaapi_drm_picture.cc:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. On 2014/11/12 05:52:58, Pawel Osciak wrote: > No "(c)" in new files please. Please use > http://www.chromium.org/developers/coding-style#TOC-File-headers for the correct > header. Acknowledged. https://codereview.chromium.org/490233002/diff/480001/content/common/gpu/medi... content/common/gpu/media/vaapi_drm_picture.cc:64: va_attrib_extbuf.pixel_format = VA_FOURCC_BGRX; On 2014/11/12 05:52:58, Pawel Osciak wrote: > Perhaps memset it to 0 first and also drop 0-assignments? Acknowledged. https://codereview.chromium.org/490233002/diff/480001/content/common/gpu/medi... content/common/gpu/media/vaapi_drm_picture.cc:86: va_surface_ = vaapi_wrapper_->CreateUnownedSurface( On 2014/11/12 05:52:58, Pawel Osciak wrote: > You can't dereference a WeakPtr like this. It will return NULL if it's no longer > valid (please see explanation at > https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/weak_p...). Acknowledged. https://codereview.chromium.org/490233002/diff/480001/content/common/gpu/medi... content/common/gpu/media/vaapi_drm_picture.cc:102: } On 2014/11/12 05:52:58, Pawel Osciak wrote: > Do we need to make context current somewhere around here? It's done line 93. https://codereview.chromium.org/490233002/diff/480001/content/common/gpu/medi... content/common/gpu/media/vaapi_drm_picture.cc:106: if (!egl_image_->BindTexImage(GL_TEXTURE_EXTERNAL_OES)) { On 2014/11/12 05:52:58, Pawel Osciak wrote: > Do we need to ReleaseTexImage() somewhere? I guess I can do this in the destructor of the VaapiDrmPicture, though it's a noop for GLImageEGL (and subclasses). https://codereview.chromium.org/490233002/diff/480001/content/common/gpu/medi... content/common/gpu/media/vaapi_drm_picture.cc:127: uint32 VaapiPicture::GetGLTextureTarget() { On 2014/11/12 05:52:58, Pawel Osciak wrote: > I feel this is not a good way to do this. Please make it non-static and > implement in VaapiDrmPicture and TFP. We need to know the texture target for the user prior creation of the pictures. Should I move this into the VDA? https://codereview.chromium.org/490233002/diff/480001/content/common/gpu/medi... File content/common/gpu/media/vaapi_drm_picture.h (right): https://codereview.chromium.org/490233002/diff/480001/content/common/gpu/medi... content/common/gpu/media/vaapi_drm_picture.h:15: #include "third_party/libva/va/va.h" On 2014/11/12 05:52:58, Pawel Osciak wrote: > This include should probably be in the .cc file. Acknowledged. https://codereview.chromium.org/490233002/diff/480001/content/common/gpu/medi... File content/common/gpu/media/vaapi_h264_decoder_unittest.cc (right): https://codereview.chromium.org/490233002/diff/480001/content/common/gpu/medi... content/common/gpu/media/vaapi_h264_decoder_unittest.cc:84: scoped_ptr<VaapiWrapper> wrapper_; On 2014/11/12 05:52:58, Pawel Osciak wrote: > Please add a comment about the order. Acknowledged. https://codereview.chromium.org/490233002/diff/480001/content/common/gpu/medi... File content/common/gpu/media/vaapi_picture.h (right): https://codereview.chromium.org/490233002/diff/480001/content/common/gpu/medi... content/common/gpu/media/vaapi_picture.h:42: // Returns the type of GL texture used bind images (either On 2014/11/12 05:52:58, Pawel Osciak wrote: > Please rephrase... Acknowledged. https://codereview.chromium.org/490233002/diff/480001/content/common/gpu/medi... File content/common/gpu/media/vaapi_tfp_picture.cc (right): https://codereview.chromium.org/490233002/diff/480001/content/common/gpu/medi... content/common/gpu/media/vaapi_tfp_picture.cc:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. On 2014/11/12 05:52:58, Pawel Osciak wrote: > no "(c)" please. Acknowledged. https://codereview.chromium.org/490233002/diff/480001/content/common/gpu/medi... File content/common/gpu/media/vaapi_tfp_picture.h (right): https://codereview.chromium.org/490233002/diff/480001/content/common/gpu/medi... content/common/gpu/media/vaapi_tfp_picture.h:15: #include "third_party/libva/va/va.h" On 2014/11/12 05:52:58, Pawel Osciak wrote: > Not needed? Acknowledged. https://codereview.chromium.org/490233002/diff/480001/content/common/gpu/medi... File content/common/gpu/media/vaapi_video_decode_accelerator.cc (right): https://codereview.chromium.org/490233002/diff/480001/content/common/gpu/medi... content/common/gpu/media/vaapi_video_decode_accelerator.cc:101: // These references are destroyed when we call vaTerminate(), this On 2014/11/12 05:52:58, Pawel Osciak wrote: > Could you identify the calls that destroy them? Could we move them to the end of > vaPutSurface instead? > > Otherwise, I think then maybe we'd have to make VaapiWrapper the producer and > owner of these until we fix this issue. On destruction, it would have to do the > dance in the correct order. It's preferable not to concern VAVDA with this. > > But in any case, could you please point to the place in libva that gets and puts > the reference? I'd prefer to move the put call to end of vaPutSurface, we could > have a local patch in our repo for now. LibVA maintains a hash table of X-Pixmap -> DRI2Drawable internally. Upon vaTerminate() call, this hash table is destroyed (http://cgit.freedesktop.org/libva/tree/va/x11/va_dricommon.c#n98), that triggers a DRI2DestroyDrawable call for each DRI2Drawable handle (http://cgit.freedesktop.org/libva/tree/va/x11/va_dri2.c#n228). My understanding is that this table is there to avoid calls to the X server everytime we call vaPutSurface(). I suppose we could be patch LibVA to avoid the destruction of the DRIDrawable given that we know we will destroy X pixmap. Let me know if you agree with this.
https://codereview.chromium.org/490233002/diff/480001/content/common/gpu/medi... File content/common/gpu/media/vaapi_video_decode_accelerator.cc (right): https://codereview.chromium.org/490233002/diff/480001/content/common/gpu/medi... content/common/gpu/media/vaapi_video_decode_accelerator.cc:101: // These references are destroyed when we call vaTerminate(), this On 2014/11/12 18:04:43, llandwerlin wrote: > On 2014/11/12 05:52:58, Pawel Osciak wrote: > > Could you identify the calls that destroy them? Could we move them to the end > of > > vaPutSurface instead? > > > > Otherwise, I think then maybe we'd have to make VaapiWrapper the producer and > > owner of these until we fix this issue. On destruction, it would have to do > the > > dance in the correct order. It's preferable not to concern VAVDA with this. > > > > But in any case, could you please point to the place in libva that gets and > puts > > the reference? I'd prefer to move the put call to end of vaPutSurface, we > could > > have a local patch in our repo for now. > > LibVA maintains a hash table of X-Pixmap -> DRI2Drawable internally. > Upon vaTerminate() call, this hash table is destroyed > (http://cgit.freedesktop.org/libva/tree/va/x11/va_dricommon.c#n98), that > triggers a DRI2DestroyDrawable call for each DRI2Drawable handle > (http://cgit.freedesktop.org/libva/tree/va/x11/va_dri2.c#n228). > > My understanding is that this table is there to avoid calls to the X server > everytime we call vaPutSurface(). > I suppose we could be patch LibVA to avoid the destruction of the DRIDrawable > given that we know we will destroy X pixmap. > > Let me know if you agree with this. I don't think I agree. A DRIDrawable is basically wrapping around a Pixmap (and gets a reference to it). Since in the X server the DRIDrawable has a reference to the buffer, you can free the DRIDrawable and the Pixmap independently. Now the part that I don't understand is, what generates BadDrawable errors? Is it vaTerminate itself? Or are we calling something else after vaTerminate (which would be a bug here)? https://codereview.chromium.org/490233002/diff/520001/content/common/gpu/medi... File content/common/gpu/media/vaapi_tfp_picture.cc (right): https://codereview.chromium.org/490233002/diff/520001/content/common/gpu/medi... content/common/gpu/media/vaapi_tfp_picture.cc:42: XSync(x_display_, False); // Needed to work around buggy vdpau-driver. Hmm, why do we need that at all? we don't support vdpau anyway and it will create lag when closing videos.
marcheu@chromium.org changed reviewers: + piman@chromium.org
https://codereview.chromium.org/490233002/diff/480001/content/common/gpu/medi... File content/common/gpu/media/vaapi_video_decode_accelerator.cc (right): https://codereview.chromium.org/490233002/diff/480001/content/common/gpu/medi... content/common/gpu/media/vaapi_video_decode_accelerator.cc:101: // These references are destroyed when we call vaTerminate(), this On 2014/11/14 19:35:47, marcheu wrote: > On 2014/11/12 18:04:43, llandwerlin wrote: > > On 2014/11/12 05:52:58, Pawel Osciak wrote: > > > Could you identify the calls that destroy them? Could we move them to the > end > > of > > > vaPutSurface instead? > > > > > > Otherwise, I think then maybe we'd have to make VaapiWrapper the producer > and > > > owner of these until we fix this issue. On destruction, it would have to do > > the > > > dance in the correct order. It's preferable not to concern VAVDA with this. > > > > > > But in any case, could you please point to the place in libva that gets and > > puts > > > the reference? I'd prefer to move the put call to end of vaPutSurface, we > > could > > > have a local patch in our repo for now. > > > > LibVA maintains a hash table of X-Pixmap -> DRI2Drawable internally. > > Upon vaTerminate() call, this hash table is destroyed > > (http://cgit.freedesktop.org/libva/tree/va/x11/va_dricommon.c#n98), that > > triggers a DRI2DestroyDrawable call for each DRI2Drawable handle > > (http://cgit.freedesktop.org/libva/tree/va/x11/va_dri2.c#n228). > > > > My understanding is that this table is there to avoid calls to the X server > > everytime we call vaPutSurface(). > > I suppose we could be patch LibVA to avoid the destruction of the DRIDrawable > > given that we know we will destroy X pixmap. > > > > Let me know if you agree with this. > > I don't think I agree. A DRIDrawable is basically wrapping around a Pixmap (and > gets a reference to it). Since in the X server the DRIDrawable has a reference > to the buffer, you can free the DRIDrawable and the Pixmap independently. > > Now the part that I don't understand is, what generates BadDrawable errors? Is > it vaTerminate itself? Or are we calling something else after vaTerminate (which > would be a bug here)? > From what I can see in the DRI2 protocol, there are CreateDrawable and DestroyDrawable events. What seems misleading to me is that you create and destroy resources for which the client doesn't get a particular ID. The only way to talk about these things to the X server is through the X Pixmap ID. Now both Mesa : http://cgit.freedesktop.org/mesa/mesa/tree/src/glx/dri2.c#n371 and LibVA : http://cgit.freedesktop.org/vaapi/libva/tree/va/x11/va_dri2.c#n212 manipulate the DRI2Drawables through the pixmap ID. On X11 backed chromium, what we currently (ie. before this CL) do when terminating the VDA is the following : 1. terminate libva, which destroys libva's internal DRI2Drawable 2. destroy the TFPPicture, which destroys the GLXPixmap and the X Pixmap We cannot inverse the order here, because if we terminate libva after the X Pixmap is destroyed, we get BadDrawable errors when libva tries to destroy its own DRI2Drawables (rightfully so, because the referring X Pixmap doesn't exist anymore). With the new DrmPicture code, we create an EGLimage and a VASurface out of the same Ozone NativePixmap, and we destroy the VASurface, the EGLImage and finally the NativePixmap before we terminate libva. But in this case it doesn't matter, before libva isn't going to destroy anything related to these resources when we call vaTerminate(). So far this multiple steps destroying sequence is the only way I managed to work around everybody's different dependencies on resources. I believe the DRI2DestroyDrawable message sent by libva to the X server could be removed, because we know at the point that the pixmap is gone. https://codereview.chromium.org/490233002/diff/520001/content/common/gpu/medi... File content/common/gpu/media/vaapi_tfp_picture.cc (right): https://codereview.chromium.org/490233002/diff/520001/content/common/gpu/medi... content/common/gpu/media/vaapi_tfp_picture.cc:42: XSync(x_display_, False); // Needed to work around buggy vdpau-driver. On 2014/11/14 19:35:47, marcheu wrote: > Hmm, why do we need that at all? we don't support vdpau anyway and it will > create lag when closing videos. I don't know either. I just kept the destruction sequence as it was before, thinking that maybe you would have an internal vdpau driver supporting the libva api (since I couldn't find a recent enough opensource version). Since you mentioned vdpau is not supported, I will delete.
marcheu@chromium.org changed reviewers: + gb.devel@gmail.com
https://codereview.chromium.org/490233002/diff/480001/content/common/gpu/medi... File content/common/gpu/media/vaapi_video_decode_accelerator.cc (right): https://codereview.chromium.org/490233002/diff/480001/content/common/gpu/medi... content/common/gpu/media/vaapi_video_decode_accelerator.cc:101: // These references are destroyed when we call vaTerminate(), this On 2014/11/15 00:15:39, llandwerlin wrote: > On 2014/11/14 19:35:47, marcheu wrote: > > On 2014/11/12 18:04:43, llandwerlin wrote: > > > On 2014/11/12 05:52:58, Pawel Osciak wrote: > > > > Could you identify the calls that destroy them? Could we move them to the > > end > > > of > > > > vaPutSurface instead? > > > > > > > > Otherwise, I think then maybe we'd have to make VaapiWrapper the producer > > and > > > > owner of these until we fix this issue. On destruction, it would have to > do > > > the > > > > dance in the correct order. It's preferable not to concern VAVDA with > this. > > > > > > > > But in any case, could you please point to the place in libva that gets > and > > > puts > > > > the reference? I'd prefer to move the put call to end of vaPutSurface, we > > > could > > > > have a local patch in our repo for now. > > > > > > LibVA maintains a hash table of X-Pixmap -> DRI2Drawable internally. > > > Upon vaTerminate() call, this hash table is destroyed > > > (http://cgit.freedesktop.org/libva/tree/va/x11/va_dricommon.c#n98), that > > > triggers a DRI2DestroyDrawable call for each DRI2Drawable handle > > > (http://cgit.freedesktop.org/libva/tree/va/x11/va_dri2.c#n228). > > > > > > My understanding is that this table is there to avoid calls to the X server > > > everytime we call vaPutSurface(). > > > I suppose we could be patch LibVA to avoid the destruction of the > DRIDrawable > > > given that we know we will destroy X pixmap. > > > > > > Let me know if you agree with this. > > > > I don't think I agree. A DRIDrawable is basically wrapping around a Pixmap > (and > > gets a reference to it). Since in the X server the DRIDrawable has a reference > > to the buffer, you can free the DRIDrawable and the Pixmap independently. > > > > Now the part that I don't understand is, what generates BadDrawable errors? Is > > it vaTerminate itself? Or are we calling something else after vaTerminate > (which > > would be a bug here)? > > > > From what I can see in the DRI2 protocol, there are CreateDrawable and > DestroyDrawable events. > What seems misleading to me is that you create and destroy resources for which > the client doesn't get a particular ID. The only way to talk about these things > to the X server is through the X Pixmap ID. > > Now both Mesa : http://cgit.freedesktop.org/mesa/mesa/tree/src/glx/dri2.c#n371 > > and LibVA : http://cgit.freedesktop.org/vaapi/libva/tree/va/x11/va_dri2.c#n212 > > manipulate the DRI2Drawables through the pixmap ID. > > On X11 backed chromium, what we currently (ie. before this CL) do when > terminating the VDA is the following : > > 1. terminate libva, which destroys libva's internal DRI2Drawable > 2. destroy the TFPPicture, which destroys the GLXPixmap and the X Pixmap > > We cannot inverse the order here, because if we terminate libva after the X > Pixmap is destroyed, we get BadDrawable errors when libva tries to destroy its > own DRI2Drawables (rightfully so, because the referring X Pixmap doesn't exist > anymore). > > With the new DrmPicture code, we create an EGLimage and a VASurface out of the > same Ozone NativePixmap, and we destroy the VASurface, the EGLImage and finally > the NativePixmap before we terminate libva. But in this case it doesn't matter, > before libva isn't going to destroy anything related to these resources when we > call vaTerminate(). > > So far this multiple steps destroying sequence is the only way I managed to work > around everybody's different dependencies on resources. > > I believe the DRI2DestroyDrawable message sent by libva to the X server could be > removed, because we know at the point that the pixmap is gone. Ah I understand now, thanks for the explanation. So, http://lists.freedesktop.org/archives/libva/2011-July/000567.html is supposed to take care of that specific problem. Why doesn't it in this case?
hshi@chromium.org changed reviewers: + hshi@chromium.org
https://codereview.chromium.org/490233002/diff/520001/ui/gl/gl_image_glx.cc File ui/gl/gl_image_glx.cc (right): https://codereview.chromium.org/490233002/diff/520001/ui/gl/gl_image_glx.cc#n... ui/gl/gl_image_glx.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. This file has been removed as of https://codereview.chromium.org/699643002
On 2014/11/18 18:24:55, hshi1 wrote: > https://codereview.chromium.org/490233002/diff/520001/ui/gl/gl_image_glx.cc > File ui/gl/gl_image_glx.cc (right): > > https://codereview.chromium.org/490233002/diff/520001/ui/gl/gl_image_glx.cc#n... > ui/gl/gl_image_glx.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights > reserved. > This file has been removed as of https://codereview.chromium.org/699643002 Yes, we discussed that earlier with David who is ok to bring that file back as part of this CL. I will add the file at the next rebase. Thanks.
On 2014/11/18 18:30:14, llandwerlin wrote: > On 2014/11/18 18:24:55, hshi1 wrote: > > https://codereview.chromium.org/490233002/diff/520001/ui/gl/gl_image_glx.cc > > File ui/gl/gl_image_glx.cc (right): > > > > > https://codereview.chromium.org/490233002/diff/520001/ui/gl/gl_image_glx.cc#n... > > ui/gl/gl_image_glx.cc:1: // Copyright (c) 2012 The Chromium Authors. All > rights > > reserved. > > This file has been removed as of https://codereview.chromium.org/699643002 > > Yes, we discussed that earlier with David who is ok to bring that file back as > part of this CL. > I will add the file at the next rebase. > Thanks. So can you answer my question about http://lists.freedesktop.org/archives/libva/2011-July/000567.html -- why doesn't that fix the issue?
On 2014/11/18 21:48:02, marcheu wrote: > On 2014/11/18 18:30:14, llandwerlin wrote: > > On 2014/11/18 18:24:55, hshi1 wrote: > > > https://codereview.chromium.org/490233002/diff/520001/ui/gl/gl_image_glx.cc > > > File ui/gl/gl_image_glx.cc (right): > > > > > > > > > https://codereview.chromium.org/490233002/diff/520001/ui/gl/gl_image_glx.cc#n... > > > ui/gl/gl_image_glx.cc:1: // Copyright (c) 2012 The Chromium Authors. All > > rights > > > reserved. > > > This file has been removed as of https://codereview.chromium.org/699643002 > > > > Yes, we discussed that earlier with David who is ok to bring that file back as > > part of this CL. > > I will add the file at the next rebase. > > Thanks. > > So can you answer my question about > http://lists.freedesktop.org/archives/libva/2011-July/000567.html -- why doesn't > that fix the issue? I haven't had time to look into this yet. Hopefully I will come back with an answer before the end of the week. Thanks for pointing that out.
On 2014/11/18 21:48:02, marcheu wrote: > On 2014/11/18 18:30:14, llandwerlin wrote: > > On 2014/11/18 18:24:55, hshi1 wrote: > > > https://codereview.chromium.org/490233002/diff/520001/ui/gl/gl_image_glx.cc > > > File ui/gl/gl_image_glx.cc (right): > > > > > > > > > https://codereview.chromium.org/490233002/diff/520001/ui/gl/gl_image_glx.cc#n... > > > ui/gl/gl_image_glx.cc:1: // Copyright (c) 2012 The Chromium Authors. All > > rights > > > reserved. > > > This file has been removed as of https://codereview.chromium.org/699643002 > > > > Yes, we discussed that earlier with David who is ok to bring that file back as > > part of this CL. > > I will add the file at the next rebase. > > Thanks. > > So can you answer my question about > http://lists.freedesktop.org/archives/libva/2011-July/000567.html -- why doesn't > that fix the issue? Reading the libX11 code, extensions' error handlers are only called if you are waiting for an answer from the Xserver following a call that you just made. (specified by the third argument to handle_response() : http://cgit.freedesktop.org/xorg/lib/libX11/tree/src/xcb_io.c#n310 and propagated to handle_error() ) VA_DRI2Authentificate for example, waits for a response and does something like this (with _XReply) : http://cgit.freedesktop.org/libva/tree/va/x11/va_dri2.c#n185 VA_DRI2DestroyDrawable doesn't and it seems the reason for that is that the DRI2 protocol doesn't specify an answer for that call. Therefore we can't wait for anything. So I suspect the fix you're pointing at doesn't really work.
https://codereview.chromium.org/490233002/diff/520001/content/common/gpu/medi... File content/common/gpu/media/vaapi_drm_picture.cc (right): https://codereview.chromium.org/490233002/diff/520001/content/common/gpu/medi... content/common/gpu/media/vaapi_drm_picture.cc:41: DCHECK_EQ(glGetError(), GL_NO_ERROR); nit: warning: comparison between signed and unsigned integer expressions. Use "DCHECK_EQ(glGetError(), static_cast<GLenum>(GL_NO_ERROR));"
https://codereview.chromium.org/490233002/diff/520001/content/common/gpu/medi... File content/common/gpu/media/vaapi_drm_picture.cc (right): https://codereview.chromium.org/490233002/diff/520001/content/common/gpu/medi... content/common/gpu/media/vaapi_drm_picture.cc:53: factory->CreateNativePixmap(size(), ui::SurfaceFactoryOzone::RGBA_8888); need rebase - this function now takes 4 arguments
Question on testing: how do you verify that VAAPI VDA/VEA work with ozone? So far I'm not able to get VAAPI VDA/VEA to work on link_freon with this change.
On 2014/11/22 00:54:19, hshi1 wrote: > Question on testing: how do you verify that VAAPI VDA/VEA work with ozone? So > far I'm not able to get VAAPI VDA/VEA to work on link_freon with this change. I'm building inside the Chromium chroot, I'm able to run VDA/VEA unit tests on both X11 and Ozone builds as well as chromium itself. I've also tested this CL on 3 Intel platforms, SandyBridge, IvyBridge and Haswell. It's possible this CL doesn't apply on ToT, I'm waiting for Pawel to give me a green light to rebase.
On 2014/11/22 00:54:19, hshi1 wrote: > Question on testing: how do you verify that VAAPI VDA/VEA work with ozone? So > far I'm not able to get VAAPI VDA/VEA to work on link_freon with this change. I'm building inside the Chromium chroot, I'm able to run VDA/VEA unit tests on both X11 and Ozone builds as well as chromium itself. I've also tested this CL on 3 Intel platforms, SandyBridge, IvyBridge and Haswell. It's possible this CL doesn't apply on ToT, I'm waiting for Pawel to give me a green light to rebase.
On 2014/11/22 00:54:19, hshi1 wrote: > Question on testing: how do you verify that VAAPI VDA/VEA work with ozone? So > far I'm not able to get VAAPI VDA/VEA to work on link_freon with this change. I'm building inside the Chromium chroot, I'm able to run VDA/VEA unit tests on both X11 and Ozone builds as well as chromium itself. I've also tested this CL on 3 Intel platforms, SandyBridge, IvyBridge and Haswell. It's possible this CL doesn't apply on ToT, I'm waiting for Pawel to give me a green light to rebase.
On 2014/11/22 11:38:54, llandwerlin wrote: > On 2014/11/22 00:54:19, hshi1 wrote: > > Question on testing: how do you verify that VAAPI VDA/VEA work with ozone? So > > far I'm not able to get VAAPI VDA/VEA to work on link_freon with this change. > > I'm building inside the Chromium chroot, I'm able to run VDA/VEA unit tests on > both X11 and Ozone builds as well as chromium itself. > I've also tested this CL on 3 Intel platforms, SandyBridge, IvyBridge and > Haswell. > > It's possible this CL doesn't apply on ToT, I'm waiting for Pawel to give me a > green light to rebase. If possible, it would be great if we could hold off the rebase for now please, because once we solve the libva/drawables issue, we should be able to just remove the current destruction dance and the review should be quick for just that, assuming we could just diff against previous patchset. With a rebase it would be more complicated to review to be honest. Apart from the need to solve the above issue, we are looking good everywhere else I feel.
On 2014/11/22 12:31:32, Pawel Osciak wrote: > On 2014/11/22 11:38:54, llandwerlin wrote: > > On 2014/11/22 00:54:19, hshi1 wrote: > > > Question on testing: how do you verify that VAAPI VDA/VEA work with ozone? > So > > > far I'm not able to get VAAPI VDA/VEA to work on link_freon with this > change. > > > > I'm building inside the Chromium chroot, I'm able to run VDA/VEA unit tests on > > both X11 and Ozone builds as well as chromium itself. > > I've also tested this CL on 3 Intel platforms, SandyBridge, IvyBridge and > > Haswell. > > > > It's possible this CL doesn't apply on ToT, I'm waiting for Pawel to give me a > > green light to rebase. > > If possible, it would be great if we could hold off the rebase for now please, > because once we solve the libva/drawables issue, we should be able to just > remove the current destruction dance and the review should be quick for just > that, assuming we could just diff against previous patchset. With a rebase it > would be more complicated to review to be honest. > > Apart from the need to solve the above issue, we are looking good everywhere > else I feel. Pawel, Stéphane, I can't really see a simple solution here. The X connection is used from multiple thread (VDA thread as well any thread creating pixmap/window or pulling events). Because of these multiples threads accessing the display, we can't really use our own X11ErrorTracker. The VDA might even be disposed before the errors come back. Again, if this destruction sequence is not acceptable, the only solution I can think of is to suppress the DRI2DestroyDrawable calls in LibVA. The life cycle of the DRI2Drawables are limited to the life cycle of the corresponding XPixmap, so it would be fine not to emit a DRI2DestroyDrawable in the case we know the XPixmap has been destroyed before. Thanks.
On 2014/11/24 17:25:09, llandwerlin wrote: > On 2014/11/22 12:31:32, Pawel Osciak wrote: > > On 2014/11/22 11:38:54, llandwerlin wrote: > > > On 2014/11/22 00:54:19, hshi1 wrote: > > > > Question on testing: how do you verify that VAAPI VDA/VEA work with ozone? > > So > > > > far I'm not able to get VAAPI VDA/VEA to work on link_freon with this > > change. > > > > > > I'm building inside the Chromium chroot, I'm able to run VDA/VEA unit tests > on > > > both X11 and Ozone builds as well as chromium itself. > > > I've also tested this CL on 3 Intel platforms, SandyBridge, IvyBridge and > > > Haswell. > > > > > > It's possible this CL doesn't apply on ToT, I'm waiting for Pawel to give me > a > > > green light to rebase. > > > > If possible, it would be great if we could hold off the rebase for now please, > > because once we solve the libva/drawables issue, we should be able to just > > remove the current destruction dance and the review should be quick for just > > that, assuming we could just diff against previous patchset. With a rebase it > > would be more complicated to review to be honest. > > > > Apart from the need to solve the above issue, we are looking good everywhere > > else I feel. > > Pawel, Stéphane, > I can't really see a simple solution here. > The X connection is used from multiple thread (VDA thread as well any thread > creating pixmap/window or pulling events). > Because of these multiples threads accessing the display, we can't really use > our own X11ErrorTracker. > The VDA might even be disposed before the errors come back. > > Again, if this destruction sequence is not acceptable, the only solution I can > think of is to suppress the DRI2DestroyDrawable calls in LibVA. > The life cycle of the DRI2Drawables are limited to the life cycle of the > corresponding XPixmap, so it would be fine not to emit a DRI2DestroyDrawable in > the case we know the XPixmap has been destroyed before. Could we do the following in libva: - before calling destroy, call getbuffers or some other dri2 entry point with a return - check and suppress the error on that - if that passes, we can destroy the dri drawable I know it's not 100% safe if you're using threaded xlib, but no one is insane enough to use threaded xlib, right? right?
On 2014/11/25 02:47:52, marcheu wrote: > On 2014/11/24 17:25:09, llandwerlin wrote: > > On 2014/11/22 12:31:32, Pawel Osciak wrote: > > > On 2014/11/22 11:38:54, llandwerlin wrote: > > > > On 2014/11/22 00:54:19, hshi1 wrote: > > > > > Question on testing: how do you verify that VAAPI VDA/VEA work with > ozone? > > > So > > > > > far I'm not able to get VAAPI VDA/VEA to work on link_freon with this > > > change. > > > > > > > > I'm building inside the Chromium chroot, I'm able to run VDA/VEA unit > tests > > on > > > > both X11 and Ozone builds as well as chromium itself. > > > > I've also tested this CL on 3 Intel platforms, SandyBridge, IvyBridge and > > > > Haswell. > > > > > > > > It's possible this CL doesn't apply on ToT, I'm waiting for Pawel to give > me > > a > > > > green light to rebase. > > > > > > If possible, it would be great if we could hold off the rebase for now > please, > > > because once we solve the libva/drawables issue, we should be able to just > > > remove the current destruction dance and the review should be quick for just > > > that, assuming we could just diff against previous patchset. With a rebase > it > > > would be more complicated to review to be honest. > > > > > > Apart from the need to solve the above issue, we are looking good everywhere > > > else I feel. > > > > Pawel, Stéphane, > > I can't really see a simple solution here. > > The X connection is used from multiple thread (VDA thread as well any thread > > creating pixmap/window or pulling events). > > Because of these multiples threads accessing the display, we can't really use > > our own X11ErrorTracker. > > The VDA might even be disposed before the errors come back. > > > > Again, if this destruction sequence is not acceptable, the only solution I can > > think of is to suppress the DRI2DestroyDrawable calls in LibVA. > > The life cycle of the DRI2Drawables are limited to the life cycle of the > > corresponding XPixmap, so it would be fine not to emit a DRI2DestroyDrawable > in > > the case we know the XPixmap has been destroyed before. > > Could we do the following in libva: > - before calling destroy, call getbuffers or some other dri2 entry point with a > return > - check and suppress the error on that > - if that passes, we can destroy the dri drawable > > I know it's not 100% safe if you're using threaded xlib, but no one is insane > enough to use threaded xlib, right? right? Hmm, Antoine points out that it should be enough to call XSync after the DRI2DestroyDrawable to get the errors back. Could you try that first?
On 2014/11/25 03:50:02, marcheu wrote: > On 2014/11/25 02:47:52, marcheu wrote: > > On 2014/11/24 17:25:09, llandwerlin wrote: > > > On 2014/11/22 12:31:32, Pawel Osciak wrote: > > > > On 2014/11/22 11:38:54, llandwerlin wrote: > > > > > On 2014/11/22 00:54:19, hshi1 wrote: > > > > > > Question on testing: how do you verify that VAAPI VDA/VEA work with > > ozone? > > > > So > > > > > > far I'm not able to get VAAPI VDA/VEA to work on link_freon with this > > > > change. > > > > > > > > > > I'm building inside the Chromium chroot, I'm able to run VDA/VEA unit > > tests > > > on > > > > > both X11 and Ozone builds as well as chromium itself. > > > > > I've also tested this CL on 3 Intel platforms, SandyBridge, IvyBridge > and > > > > > Haswell. > > > > > > > > > > It's possible this CL doesn't apply on ToT, I'm waiting for Pawel to > give > > me > > > a > > > > > green light to rebase. > > > > > > > > If possible, it would be great if we could hold off the rebase for now > > please, > > > > because once we solve the libva/drawables issue, we should be able to just > > > > remove the current destruction dance and the review should be quick for > just > > > > that, assuming we could just diff against previous patchset. With a rebase > > it > > > > would be more complicated to review to be honest. > > > > > > > > Apart from the need to solve the above issue, we are looking good > everywhere > > > > else I feel. > > > > > > Pawel, Stéphane, > > > I can't really see a simple solution here. > > > The X connection is used from multiple thread (VDA thread as well any thread > > > creating pixmap/window or pulling events). > > > Because of these multiples threads accessing the display, we can't really > use > > > our own X11ErrorTracker. > > > The VDA might even be disposed before the errors come back. > > > > > > Again, if this destruction sequence is not acceptable, the only solution I > can > > > think of is to suppress the DRI2DestroyDrawable calls in LibVA. > > > The life cycle of the DRI2Drawables are limited to the life cycle of the > > > corresponding XPixmap, so it would be fine not to emit a DRI2DestroyDrawable > > in > > > the case we know the XPixmap has been destroyed before. > > > > Could we do the following in libva: > > - before calling destroy, call getbuffers or some other dri2 entry point with > a > > return > > - check and suppress the error on that > > - if that passes, we can destroy the dri drawable > > > > I know it's not 100% safe if you're using threaded xlib, but no one is insane > > enough to use threaded xlib, right? right? > > Hmm, Antoine points out that it should be enough to call XSync after the > DRI2DestroyDrawable to get the errors back. Could you try that first? Just tried that, it doesn't work. The errors are being pulled from the GL/UI thread (in the unit test). LibVA takes a lock on the display for each of the DRI2DestroyDrawable calls and but also releases it after each message is sent, leaving a chance for the GL thread to step in and pull some events/errors. Also tried your other solution, which works. Going with this. Thanks for the feedback.
On 2014/11/25 15:51:08, llandwerlin wrote: > On 2014/11/25 03:50:02, marcheu wrote: > > On 2014/11/25 02:47:52, marcheu wrote: > > > On 2014/11/24 17:25:09, llandwerlin wrote: > > > > On 2014/11/22 12:31:32, Pawel Osciak wrote: > > > > > On 2014/11/22 11:38:54, llandwerlin wrote: > > > > > > On 2014/11/22 00:54:19, hshi1 wrote: > > > > > > > Question on testing: how do you verify that VAAPI VDA/VEA work with > > > ozone? > > > > > So > > > > > > > far I'm not able to get VAAPI VDA/VEA to work on link_freon with > this > > > > > change. > > > > > > > > > > > > I'm building inside the Chromium chroot, I'm able to run VDA/VEA unit > > > tests > > > > on > > > > > > both X11 and Ozone builds as well as chromium itself. > > > > > > I've also tested this CL on 3 Intel platforms, SandyBridge, IvyBridge > > and > > > > > > Haswell. > > > > > > > > > > > > It's possible this CL doesn't apply on ToT, I'm waiting for Pawel to > > give > > > me > > > > a > > > > > > green light to rebase. > > > > > > > > > > If possible, it would be great if we could hold off the rebase for now > > > please, > > > > > because once we solve the libva/drawables issue, we should be able to > just > > > > > remove the current destruction dance and the review should be quick for > > just > > > > > that, assuming we could just diff against previous patchset. With a > rebase > > > it > > > > > would be more complicated to review to be honest. > > > > > > > > > > Apart from the need to solve the above issue, we are looking good > > everywhere > > > > > else I feel. > > > > > > > > Pawel, Stéphane, > > > > I can't really see a simple solution here. > > > > The X connection is used from multiple thread (VDA thread as well any > thread > > > > creating pixmap/window or pulling events). > > > > Because of these multiples threads accessing the display, we can't really > > use > > > > our own X11ErrorTracker. > > > > The VDA might even be disposed before the errors come back. > > > > > > > > Again, if this destruction sequence is not acceptable, the only solution I > > can > > > > think of is to suppress the DRI2DestroyDrawable calls in LibVA. > > > > The life cycle of the DRI2Drawables are limited to the life cycle of the > > > > corresponding XPixmap, so it would be fine not to emit a > DRI2DestroyDrawable > > > in > > > > the case we know the XPixmap has been destroyed before. > > > > > > Could we do the following in libva: > > > - before calling destroy, call getbuffers or some other dri2 entry point > with > > a > > > return > > > - check and suppress the error on that > > > - if that passes, we can destroy the dri drawable > > > > > > I know it's not 100% safe if you're using threaded xlib, but no one is > insane > > > enough to use threaded xlib, right? right? > > > > Hmm, Antoine points out that it should be enough to call XSync after the > > DRI2DestroyDrawable to get the errors back. Could you try that first? > > Just tried that, it doesn't work. > The errors are being pulled from the GL/UI thread (in the unit test). > > LibVA takes a lock on the display for each of the DRI2DestroyDrawable calls and > but also releases it after each message is sent, leaving a chance for the GL > thread to step in and pull some events/errors. > > Also tried your other solution, which works. Going with this. This is great news! What is your plan, submit to libva and have us pull back a patch into cros, or submit a patch to cros as well? Thanks!
On 2014/11/26 12:15:35, Pawel Osciak wrote: > On 2014/11/25 15:51:08, llandwerlin wrote: > > On 2014/11/25 03:50:02, marcheu wrote: > > > On 2014/11/25 02:47:52, marcheu wrote: > > > > On 2014/11/24 17:25:09, llandwerlin wrote: > > > > > On 2014/11/22 12:31:32, Pawel Osciak wrote: > > > > > > On 2014/11/22 11:38:54, llandwerlin wrote: > > > > > > > On 2014/11/22 00:54:19, hshi1 wrote: > > > > > > > > Question on testing: how do you verify that VAAPI VDA/VEA work > with > > > > ozone? > > > > > > So > > > > > > > > far I'm not able to get VAAPI VDA/VEA to work on link_freon with > > this > > > > > > change. > > > > > > > > > > > > > > I'm building inside the Chromium chroot, I'm able to run VDA/VEA > unit > > > > tests > > > > > on > > > > > > > both X11 and Ozone builds as well as chromium itself. > > > > > > > I've also tested this CL on 3 Intel platforms, SandyBridge, > IvyBridge > > > and > > > > > > > Haswell. > > > > > > > > > > > > > > It's possible this CL doesn't apply on ToT, I'm waiting for Pawel to > > > give > > > > me > > > > > a > > > > > > > green light to rebase. > > > > > > > > > > > > If possible, it would be great if we could hold off the rebase for now > > > > please, > > > > > > because once we solve the libva/drawables issue, we should be able to > > just > > > > > > remove the current destruction dance and the review should be quick > for > > > just > > > > > > that, assuming we could just diff against previous patchset. With a > > rebase > > > > it > > > > > > would be more complicated to review to be honest. > > > > > > > > > > > > Apart from the need to solve the above issue, we are looking good > > > everywhere > > > > > > else I feel. > > > > > > > > > > Pawel, Stéphane, > > > > > I can't really see a simple solution here. > > > > > The X connection is used from multiple thread (VDA thread as well any > > thread > > > > > creating pixmap/window or pulling events). > > > > > Because of these multiples threads accessing the display, we can't > really > > > use > > > > > our own X11ErrorTracker. > > > > > The VDA might even be disposed before the errors come back. > > > > > > > > > > Again, if this destruction sequence is not acceptable, the only solution > I > > > can > > > > > think of is to suppress the DRI2DestroyDrawable calls in LibVA. > > > > > The life cycle of the DRI2Drawables are limited to the life cycle of the > > > > > corresponding XPixmap, so it would be fine not to emit a > > DRI2DestroyDrawable > > > > in > > > > > the case we know the XPixmap has been destroyed before. > > > > > > > > Could we do the following in libva: > > > > - before calling destroy, call getbuffers or some other dri2 entry point > > with > > > a > > > > return > > > > - check and suppress the error on that > > > > - if that passes, we can destroy the dri drawable > > > > > > > > I know it's not 100% safe if you're using threaded xlib, but no one is > > insane > > > > enough to use threaded xlib, right? right? > > > > > > Hmm, Antoine points out that it should be enough to call XSync after the > > > DRI2DestroyDrawable to get the errors back. Could you try that first? > > > > Just tried that, it doesn't work. > > The errors are being pulled from the GL/UI thread (in the unit test). > > > > LibVA takes a lock on the display for each of the DRI2DestroyDrawable calls > and > > but also releases it after each message is sent, leaving a chance for the GL > > thread to step in and pull some events/errors. > > > > Also tried your other solution, which works. Going with this. > > This is great news! What is your plan, submit to libva and have us pull back a > patch into cros, or submit a patch to cros as well? > Thanks! I'm about to submit the change to LibVA. But I've also spotted a problem with dmabufs in a recent upload of Mesa in cros : https://code.google.com/p/chromium/issues/detail?id=436842
On 2014/11/26 12:21:53, llandwerlin wrote: > On 2014/11/26 12:15:35, Pawel Osciak wrote: > > On 2014/11/25 15:51:08, llandwerlin wrote: > > > On 2014/11/25 03:50:02, marcheu wrote: > > > > On 2014/11/25 02:47:52, marcheu wrote: > > > > > On 2014/11/24 17:25:09, llandwerlin wrote: > > > > > > On 2014/11/22 12:31:32, Pawel Osciak wrote: > > > > > > > On 2014/11/22 11:38:54, llandwerlin wrote: > > > > > > > > On 2014/11/22 00:54:19, hshi1 wrote: > > > > > > > > > Question on testing: how do you verify that VAAPI VDA/VEA work > > with > > > > > ozone? > > > > > > > So > > > > > > > > > far I'm not able to get VAAPI VDA/VEA to work on link_freon with > > > this > > > > > > > change. > > > > > > > > > > > > > > > > I'm building inside the Chromium chroot, I'm able to run VDA/VEA > > unit > > > > > tests > > > > > > on > > > > > > > > both X11 and Ozone builds as well as chromium itself. > > > > > > > > I've also tested this CL on 3 Intel platforms, SandyBridge, > > IvyBridge > > > > and > > > > > > > > Haswell. > > > > > > > > > > > > > > > > It's possible this CL doesn't apply on ToT, I'm waiting for Pawel > to > > > > give > > > > > me > > > > > > a > > > > > > > > green light to rebase. > > > > > > > > > > > > > > If possible, it would be great if we could hold off the rebase for > now > > > > > please, > > > > > > > because once we solve the libva/drawables issue, we should be able > to > > > just > > > > > > > remove the current destruction dance and the review should be quick > > for > > > > just > > > > > > > that, assuming we could just diff against previous patchset. With a > > > rebase > > > > > it > > > > > > > would be more complicated to review to be honest. > > > > > > > > > > > > > > Apart from the need to solve the above issue, we are looking good > > > > everywhere > > > > > > > else I feel. > > > > > > > > > > > > Pawel, Stéphane, > > > > > > I can't really see a simple solution here. > > > > > > The X connection is used from multiple thread (VDA thread as well any > > > thread > > > > > > creating pixmap/window or pulling events). > > > > > > Because of these multiples threads accessing the display, we can't > > really > > > > use > > > > > > our own X11ErrorTracker. > > > > > > The VDA might even be disposed before the errors come back. > > > > > > > > > > > > Again, if this destruction sequence is not acceptable, the only > solution > > I > > > > can > > > > > > think of is to suppress the DRI2DestroyDrawable calls in LibVA. > > > > > > The life cycle of the DRI2Drawables are limited to the life cycle of > the > > > > > > corresponding XPixmap, so it would be fine not to emit a > > > DRI2DestroyDrawable > > > > > in > > > > > > the case we know the XPixmap has been destroyed before. > > > > > > > > > > Could we do the following in libva: > > > > > - before calling destroy, call getbuffers or some other dri2 entry point > > > with > > > > a > > > > > return > > > > > - check and suppress the error on that > > > > > - if that passes, we can destroy the dri drawable > > > > > > > > > > I know it's not 100% safe if you're using threaded xlib, but no one is > > > insane > > > > > enough to use threaded xlib, right? right? > > > > > > > > Hmm, Antoine points out that it should be enough to call XSync after the > > > > DRI2DestroyDrawable to get the errors back. Could you try that first? > > > > > > Just tried that, it doesn't work. > > > The errors are being pulled from the GL/UI thread (in the unit test). > > > > > > LibVA takes a lock on the display for each of the DRI2DestroyDrawable calls > > and > > > but also releases it after each message is sent, leaving a chance for the GL > > > thread to step in and pull some events/errors. > > > > > > Also tried your other solution, which works. Going with this. > > > > This is great news! What is your plan, submit to libva and have us pull back a > > patch into cros, or submit a patch to cros as well? > > Thanks! > > I'm about to submit the change to LibVA. > But I've also spotted a problem with dmabufs in a recent upload of Mesa in cros > : https://code.google.com/p/chromium/issues/detail?id=436842 Yes, we are aware of this, sorry about that. It's unintentional, we were a bit overzealous in reverts. I'm planning to submit patches correcting this. Please feel free to test with those revert patches disabled for now.
PTAL, Thanks. Here is the LibVA patch I'm using for X11 : http://lists.freedesktop.org/archives/libva/2014-November/002901.html
On 2014/11/26 13:07:02, llandwerlin wrote: > PTAL, Thanks. > > Here is the LibVA patch I'm using for X11 : > http://lists.freedesktop.org/archives/libva/2014-November/002901.html Thanks, started the review. I saw some comments on the patch to libva, do you think your new version will be accepted?
On 2014/12/02 02:18:42, Pawel Osciak wrote: > On 2014/11/26 13:07:02, llandwerlin wrote: > > PTAL, Thanks. > > > > Here is the LibVA patch I'm using for X11 : > > http://lists.freedesktop.org/archives/libva/2014-November/002901.html > > Thanks, started the review. I saw some comments on the patch to libva, do you > think your new version will be accepted? I'm trying to get hold of the maintainer, but I can tell other people are having the same problem, so I'm pretty confident if not this patch, another one will be included to solve that issue.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 I will merge it later this week. Haihao and I have been busy. Sean On 12/02/2014 10:43 AM, lionel.g.landwerlin@intel.com wrote: > On 2014/12/02 02:18:42, Pawel Osciak wrote: >> On 2014/11/26 13:07:02, llandwerlin wrote: >>> PTAL, Thanks. >>> >>> Here is the LibVA patch I'm using for X11 : >>> http://lists.freedesktop.org/archives/libva/2014-November/002901.html > >>> >> Thanks, started the review. I saw some comments on the patch to >> libva, do you think your new version will be accepted? > > I'm trying to get hold of the maintainer, but I can tell other > people are having the same problem, so I'm pretty confident if not > this patch, another one will be included to solve that issue. > > https://codereview.chromium.org/490233002/ - -- Sean V. Kelley <sean.v.kelley@intel.com> Open Source Technology Center / SSG Intel Corp. -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJUfi1pAAoJEEScsp9M+IBYYV4QAN6D+LexmGVu3ZL9chKAlBnF itZGlJ0XANyaCKdjrWT30GWG/TKfdSXb6Qi7i76mX27ttDGLPFHuoTuTUfj7jMgw PLkfgyyn5s+w9Aq6d1nK1XACvWmNE3voJK4yuaB3C8YRmz2F69IWPfCFL8fogxE6 1Wsd2XN+XCtI4hS+6DcxSaxPjb5yQM++JiYAU5b86ktdPucXV684z6dgt22H5JCM FNwWHhqrNER07lXpm99RqJaHqAUEku5mlE9xhQ9CCTbPGpqxGugU0FAoki6J6D+b UfIAQEWfl49/1pezaPpIf/Qgf75lkEKKiK8c+1Vi6HE/3KiFqouxX+JQpA21nXVW TCu8jiaeYCXeMYo8sAB4JfwXWREMf85tM/63BDdmNjMJUFZjqVZfQC2TCXIvj8a8 t1aSUTERLEGe8O0CldoXIoRICtfLs9ztVpLOJVVHsJQMxhRAmkHxx0sMmkXjYAwy gHyfv3KWO4Pp8cxi/YKnmu0xh2tU96vHdZfh7UAI5xzkB2nu8u5/vRYdbD4m5xO8 VX3VytshkKlLNv/GbIvubCwJ9tn7G87tuCdZ5dayF0KtlUeQnKoa7Suc31gm1+ss P6WzLVM9bIdfgD8CKz8GxALpscCIYkaO1WLgCiDn2i9K3k7fxgr8cYP2/Xpw2dCJ zmxms+w082trrXvC9qdY =1y6S -----END PGP SIGNATURE----- To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Thanks Sean, the patch was applied on master.
lgtm
https://codereview.chromium.org/490233002/diff/540001/content/common/gpu/medi... File content/common/gpu/media/rendering_helper.cc (right): https://codereview.chromium.org/490233002/diff/540001/content/common/gpu/medi... content/common/gpu/media/rendering_helper.cc:29: #include "ui/gl/gl_surface_glx.h" You changed the logic here and it broke the test on ARM platform. Please change it back. # if !defined(OS_WIN) && define(ARCH_CPU_X86_FAMILY) #define GL_VARIANT_GLX 1 #include "ui/gl/gl_surface_glx.h" #else #define GL_VARIANT_EGL 1 #include "ui/gl/gl_surface_egl.h" #endif https://codereview.chromium.org/490233002/diff/540001/content/common/gpu/medi... content/common/gpu/media/rendering_helper.cc:568: } What about the ozone case ? Should we reset platform_window_delegate here to free the resource ?
https://chromiumcodereview.appspot.com/490233002/diff/480001/content/common/g... File content/common/gpu/media/vaapi_drm_picture.cc (right): https://chromiumcodereview.appspot.com/490233002/diff/480001/content/common/g... content/common/gpu/media/vaapi_drm_picture.cc:102: } On 2014/11/12 18:04:42, llandwerlin wrote: > On 2014/11/12 05:52:58, Pawel Osciak wrote: > > Do we need to make context current somewhere around here? > > It's done line 93. Right, somehow missed that, sorry. https://chromiumcodereview.appspot.com/490233002/diff/480001/content/common/g... content/common/gpu/media/vaapi_drm_picture.cc:106: if (!egl_image_->BindTexImage(GL_TEXTURE_EXTERNAL_OES)) { On 2014/11/12 18:04:42, llandwerlin wrote: > On 2014/11/12 05:52:58, Pawel Osciak wrote: > > Do we need to ReleaseTexImage() somewhere? > > I guess I can do this in the destructor of the VaapiDrmPicture, though it's a > noop for GLImageEGL (and subclasses). Yes, but we should still do this. Thanks for adding. https://chromiumcodereview.appspot.com/490233002/diff/540001/content/common/B... File content/common/BUILD.gn (right): https://chromiumcodereview.appspot.com/490233002/diff/540001/content/common/B... content/common/BUILD.gn:18: } else { Do we need use_ozone here? https://chromiumcodereview.appspot.com/490233002/diff/540001/content/common/g... File content/common/gpu/media/gpu_video_encode_accelerator.cc (right): https://chromiumcodereview.appspot.com/490233002/diff/540001/content/common/g... content/common/gpu/media/gpu_video_encode_accelerator.cc:199: const CommandLine* cmd_line = base::CommandLine::ForCurrentProcess(); Please don't remove base:: qualifier. https://chromiumcodereview.appspot.com/490233002/diff/540001/content/common/g... File content/common/gpu/media/rendering_helper.cc (right): https://chromiumcodereview.appspot.com/490233002/diff/540001/content/common/g... content/common/gpu/media/rendering_helper.cc:144: static ui::UiThreadGpu ui_thread_gpu; Please make this a member of RenderingHelper::StubOzoneDelegate instead. https://chromiumcodereview.appspot.com/490233002/diff/540001/content/common/g... File content/common/gpu/media/vaapi_drm_picture.cc (right): https://chromiumcodereview.appspot.com/490233002/diff/540001/content/common/g... content/common/gpu/media/vaapi_drm_picture.cc:35: DCHECK(!vaapi_wrapper_.get()); Do we still need this check? If so, why? https://chromiumcodereview.appspot.com/490233002/diff/540001/content/common/g... content/common/gpu/media/vaapi_drm_picture.cc:94: va_surface_ = vaapi_wrapper_->CreateUnownedSurface( We can't have vaapi_wrapper_ as WeakPtr, because we have to be destroyed, always. We need to keep a refcount to it. https://chromiumcodereview.appspot.com/490233002/diff/540001/content/common/g... File content/common/gpu/media/vaapi_picture.h (right): https://chromiumcodereview.appspot.com/490233002/diff/540001/content/common/g... content/common/gpu/media/vaapi_picture.h:40: static uint32 GetGLTextureTarget(); Please instead of implementing this in each picture file, implement it in vaapi_picture.cc together with CreatePicture() (as mentioned in another comment). https://chromiumcodereview.appspot.com/490233002/diff/540001/content/common/g... File content/common/gpu/media/vaapi_tfp_picture.cc (right): https://chromiumcodereview.appspot.com/490233002/diff/540001/content/common/g... content/common/gpu/media/vaapi_tfp_picture.cc:37: DCHECK_EQ(glGetError(), static_cast<GLenum>(GL_NO_ERROR)); Is this required to compile? You didn't have it previously. Or were you testing on non-Debug build? https://chromiumcodereview.appspot.com/490233002/diff/540001/content/common/g... File content/common/gpu/media/vaapi_video_decode_accelerator.h (right): https://chromiumcodereview.appspot.com/490233002/diff/540001/content/common/g... content/common/gpu/media/vaapi_video_decode_accelerator.h:240: // Comes after |pictures_| to ensure LibVA has time to destroy s/after/before https://chromiumcodereview.appspot.com/490233002/diff/540001/content/common/g... content/common/gpu/media/vaapi_video_decode_accelerator.h:241: // references to X11 pixmaps before these are destroyed. s/these/it's/ https://chromiumcodereview.appspot.com/490233002/diff/540001/content/common/g... File content/common/gpu/media/vaapi_video_encode_accelerator.cc (right): https://chromiumcodereview.appspot.com/490233002/diff/540001/content/common/g... content/common/gpu/media/vaapi_video_encode_accelerator.cc:219: if (!vaapi_wrapper_.get()) { Is this required? https://chromiumcodereview.appspot.com/490233002/diff/540001/content/common/g... File content/common/gpu/media/vaapi_wrapper.cc (right): https://chromiumcodereview.appspot.com/490233002/diff/540001/content/common/g... content/common/gpu/media/vaapi_wrapper.cc:65: class VaapiWrapper; This is needed? https://chromiumcodereview.appspot.com/490233002/diff/540001/content/common/g... content/common/gpu/media/vaapi_wrapper.cc:179: vaapi_wrapper = NULL; please keep reset() https://chromiumcodereview.appspot.com/490233002/diff/540001/content/common/g... content/common/gpu/media/vaapi_wrapper.cc:239: ui::SurfaceFactoryOzone* factory = platform->GetSurfaceFactoryOzone(); Do we need to check these succeeded? https://chromiumcodereview.appspot.com/490233002/diff/540001/content/common/g... content/common/gpu/media/vaapi_wrapper.cc:481: linked_ptr<VaapiPicture> VaapiWrapper::CreatePicture( Please instead have CreatePicture as a static member of VaapiPicture. https://chromiumcodereview.appspot.com/490233002/diff/540001/content/common/g... content/common/gpu/media/vaapi_wrapper.cc:902: DeinitializeVpp_Locked(); This feels unnecessary. https://chromiumcodereview.appspot.com/490233002/diff/540001/content/common/g... File content/common/gpu/media/vaapi_wrapper.h (right): https://chromiumcodereview.appspot.com/490233002/diff/540001/content/common/g... content/common/gpu/media/vaapi_wrapper.h:57: ~VaapiWrapper(); Any reason to move this? https://chromiumcodereview.appspot.com/490233002/diff/540001/content/common/g... content/common/gpu/media/vaapi_wrapper.h:86: // the surface is transfered to the call. It differs from surfaces s/call/caller/ https://chromiumcodereview.appspot.com/490233002/diff/540001/content/common/g... content/common/gpu/media/vaapi_wrapper.h:175: // Put data from |va_surface_id| into |x_pixmap| of size |size|, s/size/dest_size/ https://chromiumcodereview.appspot.com/490233002/diff/540001/content/common/g... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://chromiumcodereview.appspot.com/490233002/diff/540001/content/common/g... content/common/gpu/media/video_encode_accelerator_unittest.cc:1179: ui::OzonePlatform::InitializeForUI(); Is this needed? This doesn't display anything, is this needed to create surfaces? (Sorry if I asked this before).
alexst, spang: could you guys please take another look at the ui/ changes if you are still ok with them? Thanks.
https://codereview.chromium.org/490233002/diff/540001/content/common/BUILD.gn File content/common/BUILD.gn (right): https://codereview.chromium.org/490233002/diff/540001/content/common/BUILD.gn... content/common/BUILD.gn:18: } else { On 2014/12/08 10:55:15, Pawel Osciak wrote: > Do we need use_ozone here? Done. https://codereview.chromium.org/490233002/diff/540001/content/common/gpu/medi... File content/common/gpu/media/gpu_video_encode_accelerator.cc (right): https://codereview.chromium.org/490233002/diff/540001/content/common/gpu/medi... content/common/gpu/media/gpu_video_encode_accelerator.cc:199: const CommandLine* cmd_line = base::CommandLine::ForCurrentProcess(); On 2014/12/08 10:55:15, Pawel Osciak wrote: > Please don't remove base:: qualifier. Done. https://codereview.chromium.org/490233002/diff/540001/content/common/gpu/medi... File content/common/gpu/media/rendering_helper.cc (right): https://codereview.chromium.org/490233002/diff/540001/content/common/gpu/medi... content/common/gpu/media/rendering_helper.cc:29: #include "ui/gl/gl_surface_glx.h" On 2014/12/08 09:13:34, Owen Lin wrote: > You changed the logic here and it broke the test on ARM platform. Please change > it back. > > # if !defined(OS_WIN) && define(ARCH_CPU_X86_FAMILY) > #define GL_VARIANT_GLX 1 > #include "ui/gl/gl_surface_glx.h" > #else > #define GL_VARIANT_EGL 1 > #include "ui/gl/gl_surface_egl.h" > #endif > Done. https://codereview.chromium.org/490233002/diff/540001/content/common/gpu/medi... content/common/gpu/media/rendering_helper.cc:144: static ui::UiThreadGpu ui_thread_gpu; On 2014/12/08 10:55:15, Pawel Osciak wrote: > Please make this a member of RenderingHelper::StubOzoneDelegate instead. Done. https://codereview.chromium.org/490233002/diff/540001/content/common/gpu/medi... content/common/gpu/media/rendering_helper.cc:568: } On 2014/12/08 09:13:34, Owen Lin wrote: > What about the ozone case ? > Should we reset platform_window_delegate here to free the resource ? Done. https://codereview.chromium.org/490233002/diff/540001/content/common/gpu/medi... File content/common/gpu/media/vaapi_drm_picture.cc (right): https://codereview.chromium.org/490233002/diff/540001/content/common/gpu/medi... content/common/gpu/media/vaapi_drm_picture.cc:35: DCHECK(!vaapi_wrapper_.get()); On 2014/12/08 10:55:15, Pawel Osciak wrote: > Do we still need this check? If so, why? Removed as I switched to scoped_refptr. https://codereview.chromium.org/490233002/diff/540001/content/common/gpu/medi... content/common/gpu/media/vaapi_drm_picture.cc:94: va_surface_ = vaapi_wrapper_->CreateUnownedSurface( On 2014/12/08 10:55:15, Pawel Osciak wrote: > We can't have vaapi_wrapper_ as WeakPtr, because we have to be destroyed, > always. We need to keep a refcount to it. Done. https://codereview.chromium.org/490233002/diff/540001/content/common/gpu/medi... File content/common/gpu/media/vaapi_picture.h (right): https://codereview.chromium.org/490233002/diff/540001/content/common/gpu/medi... content/common/gpu/media/vaapi_picture.h:40: static uint32 GetGLTextureTarget(); On 2014/12/08 10:55:15, Pawel Osciak wrote: > Please instead of implementing this in each picture file, implement it in > vaapi_picture.cc together with CreatePicture() (as mentioned in another > comment). Done. https://codereview.chromium.org/490233002/diff/540001/content/common/gpu/medi... File content/common/gpu/media/vaapi_tfp_picture.cc (right): https://codereview.chromium.org/490233002/diff/540001/content/common/gpu/medi... content/common/gpu/media/vaapi_tfp_picture.cc:37: DCHECK_EQ(glGetError(), static_cast<GLenum>(GL_NO_ERROR)); On 2014/12/08 10:55:15, Pawel Osciak wrote: > Is this required to compile? You didn't have it previously. Or were you testing > on non-Debug build? Yes, I was testing on non-Debug build. https://codereview.chromium.org/490233002/diff/540001/content/common/gpu/medi... File content/common/gpu/media/vaapi_video_decode_accelerator.h (right): https://codereview.chromium.org/490233002/diff/540001/content/common/gpu/medi... content/common/gpu/media/vaapi_video_decode_accelerator.h:240: // Comes after |pictures_| to ensure LibVA has time to destroy On 2014/12/08 10:55:15, Pawel Osciak wrote: > s/after/before Done. https://codereview.chromium.org/490233002/diff/540001/content/common/gpu/medi... content/common/gpu/media/vaapi_video_decode_accelerator.h:241: // references to X11 pixmaps before these are destroyed. On 2014/12/08 10:55:15, Pawel Osciak wrote: > s/these/it's/ Done. https://codereview.chromium.org/490233002/diff/540001/content/common/gpu/medi... File content/common/gpu/media/vaapi_video_encode_accelerator.cc (right): https://codereview.chromium.org/490233002/diff/540001/content/common/gpu/medi... content/common/gpu/media/vaapi_video_encode_accelerator.cc:219: if (!vaapi_wrapper_.get()) { On 2014/12/08 10:55:15, Pawel Osciak wrote: > Is this required? Removed by using scoped_refptr. https://codereview.chromium.org/490233002/diff/540001/content/common/gpu/medi... File content/common/gpu/media/vaapi_wrapper.cc (right): https://codereview.chromium.org/490233002/diff/540001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.cc:65: class VaapiWrapper; On 2014/12/08 10:55:16, Pawel Osciak wrote: > This is needed? Removing. https://codereview.chromium.org/490233002/diff/540001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.cc:179: vaapi_wrapper = NULL; On 2014/12/08 10:55:16, Pawel Osciak wrote: > please keep reset() Done. https://codereview.chromium.org/490233002/diff/540001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.cc:239: ui::SurfaceFactoryOzone* factory = platform->GetSurfaceFactoryOzone(); I don't think this is necessary. At this point the Gpu process is running, if we get a NULL pointer here, there is something really wrong. GpuMemoryBufferFactoryOzoneNativeBuffer doesn't bother with checks : https://code.google.com/p/chromium/codesearch#chromium/src/ui/ozone/gpu/gpu_m... https://codereview.chromium.org/490233002/diff/540001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.cc:481: linked_ptr<VaapiPicture> VaapiWrapper::CreatePicture( On 2014/12/08 10:55:16, Pawel Osciak wrote: > Please instead have CreatePicture as a static member of VaapiPicture. Done. https://codereview.chromium.org/490233002/diff/540001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.cc:902: DeinitializeVpp_Locked(); On 2014/12/08 10:55:15, Pawel Osciak wrote: > This feels unnecessary. Thanks, following the changes on the initialization of the VPP, I can remove DeinitializeVpp_Locked. https://codereview.chromium.org/490233002/diff/540001/content/common/gpu/medi... File content/common/gpu/media/vaapi_wrapper.h (right): https://codereview.chromium.org/490233002/diff/540001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.h:57: ~VaapiWrapper(); On 2014/12/08 10:55:16, Pawel Osciak wrote: > Any reason to move this? Moving this back. https://codereview.chromium.org/490233002/diff/540001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.h:86: // the surface is transfered to the call. It differs from surfaces On 2014/12/08 10:55:16, Pawel Osciak wrote: > s/call/caller/ Done. https://codereview.chromium.org/490233002/diff/540001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.h:175: // Put data from |va_surface_id| into |x_pixmap| of size |size|, On 2014/12/08 10:55:16, Pawel Osciak wrote: > s/size/dest_size/ Done. https://codereview.chromium.org/490233002/diff/540001/content/common/gpu/medi... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/490233002/diff/540001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:1179: ui::OzonePlatform::InitializeForUI(); On 2014/12/08 10:55:16, Pawel Osciak wrote: > Is this needed? This doesn't display anything, is this needed to create > surfaces? (Sorry if I asked this before). We need the OzoneSurfaceFactory to be initialize in order to get the DRI file descriptor to initialize LibVA.
PTAL.
dongseong.hwang@intel.com changed reviewers: + dongseong.hwang@intel.com
https://codereview.chromium.org/490233002/diff/540001/content/common/gpu/medi... File content/common/gpu/media/vaapi_drm_picture.cc (right): https://codereview.chromium.org/490233002/diff/540001/content/common/gpu/medi... content/common/gpu/media/vaapi_drm_picture.cc:106: gfx::GpuMemoryBuffer::BGRA_8888, Why is BGRA needed? GPU command buffer thinks the texture format is RGBA.
lgtm
lgtm % nits Wu-Cheng is verifying this and will give final lgtm after finishing. https://chromiumcodereview.appspot.com/490233002/diff/540001/content/common/g... File content/common/gpu/media/vaapi_video_encode_accelerator.cc (right): https://chromiumcodereview.appspot.com/490233002/diff/540001/content/common/g... content/common/gpu/media/vaapi_video_encode_accelerator.cc:219: if (!vaapi_wrapper_.get()) { On 2014/12/08 16:42:06, llandwerlin wrote: > On 2014/12/08 10:55:15, Pawel Osciak wrote: > > Is this required? > > Removed by using scoped_refptr. What I meant was, do we need to add '.get()'. https://chromiumcodereview.appspot.com/490233002/diff/540001/content/common/g... File content/common/gpu/media/vaapi_wrapper.cc (right): https://chromiumcodereview.appspot.com/490233002/diff/540001/content/common/g... content/common/gpu/media/vaapi_wrapper.cc:179: vaapi_wrapper = NULL; On 2014/12/08 16:42:07, llandwerlin wrote: > On 2014/12/08 10:55:16, Pawel Osciak wrote: > > please keep reset() > > Done. Did you miss it? https://chromiumcodereview.appspot.com/490233002/diff/560001/content/common/g... File content/common/gpu/media/vaapi_drm_picture.h (right): https://chromiumcodereview.appspot.com/490233002/diff/560001/content/common/g... content/common/gpu/media/vaapi_drm_picture.h:32: VaapiDrmPicture(scoped_refptr<VaapiWrapper> vaapi_wrapper, const scoped_refptr<...>& https://chromiumcodereview.appspot.com/490233002/diff/560001/content/common/g... File content/common/gpu/media/vaapi_picture.cc (right): https://chromiumcodereview.appspot.com/490233002/diff/560001/content/common/g... content/common/gpu/media/vaapi_picture.cc:19: scoped_refptr<VaapiWrapper> vaapi_wrapper, const scoped_refptr<>& please https://chromiumcodereview.appspot.com/490233002/diff/560001/content/common/g... File content/common/gpu/media/vaapi_picture.h (right): https://chromiumcodereview.appspot.com/490233002/diff/560001/content/common/g... content/common/gpu/media/vaapi_picture.h:48: static linked_ptr<VaapiPicture> CreatePicture( Can this return scoped_ptr? https://chromiumcodereview.appspot.com/490233002/diff/560001/content/common/g... File content/common/gpu/media/vaapi_tfp_picture.h (right): https://chromiumcodereview.appspot.com/490233002/diff/560001/content/common/g... content/common/gpu/media/vaapi_tfp_picture.h:30: VaapiTFPPicture(scoped_refptr<VaapiWrapper> vaapi_wrapper, const scoped_refptr<>& please. https://chromiumcodereview.appspot.com/490233002/diff/560001/content/common/g... File content/common/gpu/media/vaapi_video_decode_accelerator.h (right): https://chromiumcodereview.appspot.com/490233002/diff/560001/content/common/g... content/common/gpu/media/vaapi_video_decode_accelerator.h:240: // Comes before |pictures_| to ensure LibVA is terminated after With refcounting, I think we don't need this anymore?
Btw, I think it's a good time to please rebase and please retest rebased on your side as well Lionel. Thanks.
On 2014/12/09 01:48:28, Pawel Osciak wrote: > Btw, I think it's a good time to please rebase and please retest rebased on your > side as well Lionel. Thanks. I tested patch 28 (rebased locally) on link_freon 6556.0.0. Playing a mp4 video in chrome worked and HW decode was used. But VDA test failed. Did I miss anything? localhost tmp # ./video_decode_accelerator_unittest --gtest_filter=DecodeVariations/VideoDecodeAcceleratorParamTest.TestSimpleDecode/0 --test_video_data=test-25fps.h264:320:240:258:258:50:175:1 --ozone-platform=gbm --ozone-use-surfaceless [1209/115333:WARNING:screen_manager.cc(151)] Forcing initialization of primary display. Note: Google Test filter = DecodeVariations/VideoDecodeAcceleratorParamTest.TestSimpleDecode/0 [==========] Running 1 test from 1 test case. [----------] Global test environment set-up. [----------] 1 test from DecodeVariations/VideoDecodeAcceleratorParamTest [ RUN ] DecodeVariations/VideoDecodeAcceleratorParamTest.TestSimpleDecode/0 [1209/115334:WARNING:event_reader_libevdev_cros.cc(85)] libevdev: Event_Sync_State():419: Event_Sync_State: before 3262.748588 after 3262.748592 [1209/115334:FATAL:rendering_helper.cc(532)] Check failed: static_cast<int>(::gfx::g_current_gl_context_tls->Get()->glGetErrorFn()) == 0x0 (1286 vs. 0) test-25fps.h264 is from chrome source media/test/data.
https://chromiumcodereview.appspot.com/490233002/diff/540001/content/common/g... File content/common/gpu/media/vaapi_video_encode_accelerator.cc (right): https://chromiumcodereview.appspot.com/490233002/diff/540001/content/common/g... content/common/gpu/media/vaapi_video_encode_accelerator.cc:219: if (!vaapi_wrapper_.get()) { On 2014/12/09 01:19:25, Pawel Osciak wrote: > On 2014/12/08 16:42:06, llandwerlin wrote: > > On 2014/12/08 10:55:15, Pawel Osciak wrote: > > > Is this required? > > > > Removed by using scoped_refptr. > > What I meant was, do we need to add '.get()'. I think that was required at some point with scoped_ptr. https://chromiumcodereview.appspot.com/490233002/diff/540001/content/common/g... File content/common/gpu/media/vaapi_wrapper.cc (right): https://chromiumcodereview.appspot.com/490233002/diff/540001/content/common/g... content/common/gpu/media/vaapi_wrapper.cc:179: vaapi_wrapper = NULL; On 2014/12/09 01:19:25, Pawel Osciak wrote: > On 2014/12/08 16:42:07, llandwerlin wrote: > > On 2014/12/08 10:55:16, Pawel Osciak wrote: > > > please keep reset() > > > > Done. > > Did you miss it? There is no reset() method on scoped_refptr. Should I replace it with swap(nullptr) instead? https://chromiumcodereview.appspot.com/490233002/diff/560001/content/common/g... File content/common/gpu/media/vaapi_drm_picture.h (right): https://chromiumcodereview.appspot.com/490233002/diff/560001/content/common/g... content/common/gpu/media/vaapi_drm_picture.h:32: VaapiDrmPicture(scoped_refptr<VaapiWrapper> vaapi_wrapper, On 2014/12/09 01:19:25, Pawel Osciak wrote: > const scoped_refptr<...>& Done. https://chromiumcodereview.appspot.com/490233002/diff/560001/content/common/g... File content/common/gpu/media/vaapi_picture.cc (right): https://chromiumcodereview.appspot.com/490233002/diff/560001/content/common/g... content/common/gpu/media/vaapi_picture.cc:19: scoped_refptr<VaapiWrapper> vaapi_wrapper, On 2014/12/09 01:19:25, Pawel Osciak wrote: > const scoped_refptr<>& please Done. https://chromiumcodereview.appspot.com/490233002/diff/560001/content/common/g... File content/common/gpu/media/vaapi_picture.h (right): https://chromiumcodereview.appspot.com/490233002/diff/560001/content/common/g... content/common/gpu/media/vaapi_picture.h:48: static linked_ptr<VaapiPicture> CreatePicture( On 2014/12/09 01:19:25, Pawel Osciak wrote: > Can this return scoped_ptr? I thought the purpose of using the linked_ptr was to avoid all the Pass() calls involved with scoped_ptr and the problems related to storage inside std:: containers. I can give a try after the rebase. https://chromiumcodereview.appspot.com/490233002/diff/560001/content/common/g... File content/common/gpu/media/vaapi_tfp_picture.h (right): https://chromiumcodereview.appspot.com/490233002/diff/560001/content/common/g... content/common/gpu/media/vaapi_tfp_picture.h:30: VaapiTFPPicture(scoped_refptr<VaapiWrapper> vaapi_wrapper, On 2014/12/09 01:19:25, Pawel Osciak wrote: > const scoped_refptr<>& please. Done. https://chromiumcodereview.appspot.com/490233002/diff/560001/content/common/g... File content/common/gpu/media/vaapi_video_decode_accelerator.h (right): https://chromiumcodereview.appspot.com/490233002/diff/560001/content/common/g... content/common/gpu/media/vaapi_video_decode_accelerator.h:240: // Comes before |pictures_| to ensure LibVA is terminated after On 2014/12/09 01:19:25, Pawel Osciak wrote: > With refcounting, I think we don't need this anymore? Done.
On 2014/12/09 03:58:28, wuchengli wrote: > On 2014/12/09 01:48:28, Pawel Osciak wrote: > > Btw, I think it's a good time to please rebase and please retest rebased on > your > > side as well Lionel. Thanks. > I tested patch 28 (rebased locally) on link_freon 6556.0.0. Playing a mp4 video > in chrome worked and HW decode was used. But VDA test failed. Did I miss > anything? > > localhost tmp # ./video_decode_accelerator_unittest > --gtest_filter=DecodeVariations/VideoDecodeAcceleratorParamTest.TestSimpleDecode/0 > --test_video_data=test-25fps.h264:320:240:258:258:50:175:1 --ozone-platform=gbm > --ozone-use-surfaceless > [1209/115333:WARNING:screen_manager.cc(151)] Forcing initialization of primary > display. > Note: Google Test filter = > DecodeVariations/VideoDecodeAcceleratorParamTest.TestSimpleDecode/0 > [==========] Running 1 test from 1 test case. > [----------] Global test environment set-up. > [----------] 1 test from DecodeVariations/VideoDecodeAcceleratorParamTest > [ RUN ] DecodeVariations/VideoDecodeAcceleratorParamTest.TestSimpleDecode/0 > [1209/115334:WARNING:event_reader_libevdev_cros.cc(85)] libevdev: > Event_Sync_State():419: Event_Sync_State: before 3262.748588 after 3262.748592 > [1209/115334:FATAL:rendering_helper.cc(532)] Check failed: > static_cast<int>(::gfx::g_current_gl_context_tls->Get()->glGetErrorFn()) == 0x0 > (1286 vs. 0) > > test-25fps.h264 is from chrome source media/test/data. I can really tell where is error comes from as you appear to have changes on the rendering_helper.cc. Make sure you have the latest Mesa patches otherwise you will get errors in RenderingHelper::CreateTexture().
This is just a temporary upload, altough unit test are passing on X11, on Ozone it's crashing in a similar way to https://code.google.com/p/chromium/issues/detail?id=434115 I'm investigating this.
On 2014/12/09 18:10:24, llandwerlin wrote: > This is just a temporary upload, altough unit test are passing on X11, on Ozone > it's crashing in a similar way to > https://code.google.com/p/chromium/issues/detail?id=434115 > I'm investigating this. lgtm verified on link_freon that H264 VEA and VDA are both working when run concurrently
sorry to clarify: I verified on chrome that H264 decode and encode are actually working, but the unit test is still failing as mentioned by llandwerlin@ earlier.
https://codereview.chromium.org/490233002/diff/560001/content/common/gpu/medi... File content/common/gpu/media/vaapi_picture.h (right): https://codereview.chromium.org/490233002/diff/560001/content/common/gpu/medi... content/common/gpu/media/vaapi_picture.h:48: static linked_ptr<VaapiPicture> CreatePicture( On 2014/12/09 11:19:48, llandwerlin wrote: > On 2014/12/09 01:19:25, Pawel Osciak wrote: > > Can this return scoped_ptr? > > I thought the purpose of using the linked_ptr was to avoid all the Pass() calls > involved with scoped_ptr and the problems related to storage inside std:: > containers. > > I can give a try after the rebase. Yeah, although I was thinking to make this more generic, but nevermind, we can keep this. https://codereview.chromium.org/490233002/diff/580001/content/common/gpu/medi... File content/common/gpu/media/vaapi_drm_picture.h (right): https://codereview.chromium.org/490233002/diff/580001/content/common/gpu/medi... content/common/gpu/media/vaapi_drm_picture.h:32: VaapiDrmPicture(scoped_refptr<VaapiWrapper>& vaapi_wrapper, All these still need consts please.
re:#135 llandwerlin: have you used "stop ui" to kill chrome when you run the test "DecodeVariations/VideoDecodeAcceleratorParamTest.TestSimpleDecode/0"? When chrome is running, the test gets the following error on my setup: [1209/174720:WARNING:screen_manager.cc(151)] Forcing initialization of primary display. [1209/174720:ERROR:crtc_controller.cc(36)] Failed to modeset: error='Permission denied' crtc=19 connector=30 framebuffer_id=21842 mode=2560x1700@60 [1209/174720:ERROR:screen_manager.cc(188)] Failed to modeset controller Note: Google Test filter = DecodeVariations/VideoDecodeAcceleratorParamTest.TestSimpleDecode/0 [==========] Running 1 test from 1 test case. [----------] Global test environment set-up. [----------] 1 test from DecodeVariations/VideoDecodeAcceleratorParamTest [ RUN ] DecodeVariations/VideoDecodeAcceleratorParamTest.TestSimpleDecode/0 [1209/174720:WARNING:event_reader_libevdev_cros.cc(85)] libevdev: Event_Sync_State():419: Event_Sync_State: before 23001.87942 after 23001.87946 [1209/174720:WARNING:event_reader_libevdev_cros.cc(85)] libevdev: Event_Sync_State():419: Event_Sync_State: before 23001.89466 after 23001.89467 [1209/174721:WARNING:screen_manager.cc(37)] Display controller (crtc=19) already present. [1209/174721:ERROR:dri_gpu_platform_support.cc(204)] Not implemented reached in void ui::DriGpuPlatformSupport::OnAddGraphicsDevice(const base::FilePath&) [1209/174721:ERROR:crtc_controller.cc(36)] Failed to modeset: error='Permission denied' crtc=19 connector=30 framebuffer_id=21842 mode=2560x1700@60 [1209/174721:WARNING:crtc_controller.cc(69)] Trying to pageflip a buffer with the wrong size. Expected 233x0 got 2560x1700 for crtc=19 connector=30 [1209/174721:WARNING:crtc_controller.cc(69)] Trying to pageflip a buffer with the wrong size. Expected 233x0 got 2560x1700 for crtc=19 connector=30 But if I kill chrome first, then the test can run: [1209/174803:WARNING:screen_manager.cc(151)] Forcing initialization of primary display. Note: Google Test filter = DecodeVariations/VideoDecodeAcceleratorParamTest.TestSimpleDecode/0 [==========] Running 1 test from 1 test case. [----------] Global test environment set-up. [----------] 1 test from DecodeVariations/VideoDecodeAcceleratorParamTest [ RUN ] DecodeVariations/VideoDecodeAcceleratorParamTest.TestSimpleDecode/0 [1209/174803:WARNING:event_reader_libevdev_cros.cc(85)] libevdev: Event_Sync_State():419: Event_Sync_State: before 23043.557634 after 23043.557643 [1209/174803:WARNING:event_reader_libevdev_cros.cc(85)] libevdev: Event_Sync_State():419: Event_Sync_State: before 23043.559767 after 23043.559768 [1209/174803:WARNING:screen_manager.cc(37)] Display controller (crtc=19) already present. [1209/174803:ERROR:dri_gpu_platform_support.cc(204)] Not implemented reached in void ui::DriGpuPlatformSupport::OnAddGraphicsDevice(const base::FilePath&) libva info: VA-API version 0.35.1 libva info: va_getDriverName() returns 0 libva info: Trying to open /usr/lib64/va/drivers/i965_drv_video.so libva info: Found init function __vaDriverInit_0_35 libva info: va_openDriver() returns 0 [1209/174807:INFO:video_decode_accelerator_unittest.cc(1186)] Decoder 0 fps: 63.0999 [ OK ] DecodeVariations/VideoDecodeAcceleratorParamTest.TestSimpleDecode/0 (4469 ms) [----------] 1 test from DecodeVariations/VideoDecodeAcceleratorParamTest (4469 ms total) [----------] Global test environment tear-down [==========] 1 test from 1 test case ran. (4470 ms total) [ PASSED ] 1 test.
alexst@chromium.org changed reviewers: + sadrul@chromium.org
alexst@chromium.org changed reviewers: + scherkus@chromium.org
Adding more folks for owner reviews. +sadrul for ui/ +jln for sandbox +scherkus for media/ ping piman for content/
I applied patchset 29 and vda test could run when ui was stopped. When I ran a
single test, it passed. But when I ran multiple tests at once, it failed and got
stuck.
This passed: (replace 0 with 1 or 2 also passed)
./video_decode_accelerator_unittest
--gtest_filter=DecodeVariations/VideoDecodeAcceleratorParamTest.TestSimpleDecode/0
--test_video_data=test-25fps.h264:320:240:250:258:50:175:1 --v
This failed:
./video_decode_accelerator_unittest
--gtest_filter=DecodeVariations/VideoDecodeAcceleratorParamTest.TestSimpleDecode/*
--test_video_data=test-25fps.h264:320:240:250:258:50:175:1 --v
localhost tmp # ./video_decode_accelerator_unittest
--gtest_filter=DecodeVariations/VideoDecodeAcceleratorParamTest.TestSimpleDecode/*
--test_video_data=test-25fps.h264:320:240:250:258:50:175:1 --v
[1210/110911:WARNING:screen_manager.cc(151)] Forcing initialization of primary
display.
Note: Google Test filter =
DecodeVariations/VideoDecodeAcceleratorParamTest.TestSimpleDecode/*
[==========] Running 3 tests from 1 test case.
[----------] Global test environment set-up.
[----------] 3 tests from DecodeVariations/VideoDecodeAcceleratorParamTest
[ RUN ] DecodeVariations/VideoDecodeAcceleratorParamTest.TestSimpleDecode/0
[1210/110911:WARNING:event_reader_libevdev_cros.cc(85)] libevdev:
Event_Sync_State():419: Event_Sync_State: before 3518.258850 after 3518.258859
[1210/110911:WARNING:screen_manager.cc(37)] Display controller (crtc=19) already
present.
[1210/110911:ERROR:dri_gpu_platform_support.cc(204)] Not implemented reached in
void ui::DriGpuPlatformSupport::OnAddGraphicsDevice(const base::FilePath&)
libva info: VA-API version 0.35.1
libva info: va_getDriverName() returns 0
libva info: Trying to open /usr/lib64/va/drivers/i965_drv_video.so
libva info: Found init function __vaDriverInit_0_35
libva info: va_openDriver() returns 0
[1210/110916:INFO:video_decode_accelerator_unittest.cc(1186)] Decoder 0 fps:
55.2551
[ OK ] DecodeVariations/VideoDecodeAcceleratorParamTest.TestSimpleDecode/0
(4932 ms)
[ RUN ] DecodeVariations/VideoDecodeAcceleratorParamTest.TestSimpleDecode/1
[1210/110916:ERROR:dri_gpu_platform_support.cc(204)] Not implemented reached in
void ui::DriGpuPlatformSupport::OnAddGraphicsDevice(const base::FilePath&)
libva info: VA-API version 0.35.1
libva info: va_getDriverName() returns 0
libva info: Trying to open /usr/lib64/va/drivers/i965_drv_video.so
libva info: Found init function __vaDriverInit_0_35
libva info: va_openDriver() returns 0
[1210/110917:ERROR:gbm_buffer_base.cc(33)] Failed to register buffer
[1210/110917:ERROR:vaapi_drm_picture.cc(53)] Failed creating an Ozone
NativePixmap
[1210/110917:ERROR:vaapi_video_decode_accelerator.cc(532)] Failed assigning
picture buffer to a texture.
[1210/110917:ERROR:vaapi_video_decode_accelerator.cc(56)] Notifying of error 4
../../content/common/gpu/media/video_decode_accelerator_unittest.cc:1038:
Failure
Value of: client->decoder_deleted()
Actual: false
Expected: true
Decoder not deleted but Wait() returned 7, instead of 3
../../content/common/gpu/media/video_decode_accelerator_unittest.cc:1150:
Failure
Expected: AssertWaitForStateOrDeleted(note, clients[i], CS_FLUSHING) doesn't
generate new fatal failures in the current thread.
Actual: it does.
[ FAILED ]
DecodeVariations/VideoDecodeAcceleratorParamTest.TestSimpleDecode/1, where
GetParam() = 1, 10, 1, -1, 6, 0, 0 (337 ms)
[ RUN ] DecodeVariations/VideoDecodeAcceleratorParamTest.TestSimpleDecode/2
[1210/110917:ERROR:dri_gpu_platform_support.cc(204)] Not implemented reached in
void ui::DriGpuPlatformSupport::OnAddGraphicsDevice(const base::FilePath&)
libva info: VA-API version 0.35.1
libva info: va_getDriverName() returns 0
libva info: Trying to open /usr/lib64/va/drivers/i965_drv_video.so
libva info: Found init function __vaDriverInit_0_35
libva info: va_openDriver() returns 0
When the test was stuck, the stack of the thread was as follows.
Thread 9:
#2 0x0000555555cdf62b in
content::VaapiVideoDecodeAccelerator::FeedDecoderWithOutputSurfaces_Locked
(this=0x11cc976ad020) at
../../content/common/gpu/media/vaapi_video_decode_accelerator.cc:316
#3 0x0000555555cdfd89 in content::VaapiVideoDecodeAccelerator::DecodeTask
(this=0x11cc976ad020) at
../../content/common/gpu/media/vaapi_video_decode_accelerator.cc:377
Thread 1:
#3 0x00005555555f771f in content::(anonymous
namespace)::AssertWaitForStateOrDeleted (note=0x11cc96143b60,
client=0x11cc960fbda0, expected_state=content::(anonymous
namespace)::CS_FLUSHING)
at ../../content/common/gpu/media/video_decode_accelerator_unittest.cc:1036
#4 0x00005555555f83ec in content::(anonymous
namespace)::VideoDecodeAcceleratorParamTest_TestSimpleDecode_Test::TestBody
(this=0x11cc95e89ca0)
at ../../content/common/gpu/media/video_decode_accelerator_unittest.cc:1149
On 2014/12/10 01:50:50, hshi1 wrote: > re:#135 llandwerlin: have you used "stop ui" to kill chrome when you run the > test "DecodeVariations/VideoDecodeAcceleratorParamTest.TestSimpleDecode/0"? > Yes, I always stop ui before running the test.
On 2014/12/10 11:27:02, llandwerlin wrote: > On 2014/12/10 01:50:50, hshi1 wrote: > > re:#135 llandwerlin: have you used "stop ui" to kill chrome when you run the > > test "DecodeVariations/VideoDecodeAcceleratorParamTest.TestSimpleDecode/0"? > > > > Yes, I always stop ui before running the test. Lionel, can you run all the tests in the test together in one run, i.e. without passing --gtest_filter? Wu-Cheng can only run one and then the errors above appear.
On 2014/12/10 11:47:16, Pawel Osciak wrote: > On 2014/12/10 11:27:02, llandwerlin wrote: > > On 2014/12/10 01:50:50, hshi1 wrote: > > > re:#135 llandwerlin: have you used "stop ui" to kill chrome when you run the > > > test "DecodeVariations/VideoDecodeAcceleratorParamTest.TestSimpleDecode/0"? > > > > > > > Yes, I always stop ui before running the test. > > Lionel, can you run all the tests in the test together in one run, i.e. without > passing --gtest_filter? Wu-Cheng can only run one and then the errors above > appear. Not yet, it was working before the rebase. It seems something is going wrong in libdrm, a disposed buffer object is being reused, causing the kernel to report an error when calling addfb ;ater on.
PTAL, unit tests are passing again. It was an issue related to clever buffer recycling mechanism in place in libdrm, and how we bypass the libdrm to create DMABUFs.
mostly nits on virtual/override (I think we clarified the coding policy as you were working on this, so apologies for that!) biggest question is about the introduction of ref-counting aside from that I'll defer to posciak@ for the detailed / final media review https://codereview.chromium.org/490233002/diff/620001/content/common/gpu/medi... File content/common/gpu/media/rendering_helper.cc (right): https://codereview.chromium.org/490233002/diff/620001/content/common/gpu/medi... content/common/gpu/media/rendering_helper.cc:80: virtual ~StubOzoneDelegate() {} IIRC this should be override and we can drop virtual from the rest (we only need one specifier) https://codereview.chromium.org/490233002/diff/620001/content/common/gpu/medi... content/common/gpu/media/rendering_helper.cc:103: gfx::AcceleratedWidget GetAcceleratedWidget() const { return widget_; } nit: for these types of accessors they should be unix_hacker() style (similar to platform_window() below) https://codereview.chromium.org/490233002/diff/620001/content/common/gpu/medi... content/common/gpu/media/rendering_helper.cc:113: }; DISALLOW etc... https://codereview.chromium.org/490233002/diff/620001/content/common/gpu/medi... File content/common/gpu/media/vaapi_drm_picture.h (right): https://codereview.chromium.org/490233002/diff/620001/content/common/gpu/medi... content/common/gpu/media/vaapi_drm_picture.h:38: virtual ~VaapiDrmPicture(); s/virtual/override/ https://codereview.chromium.org/490233002/diff/620001/content/common/gpu/medi... content/common/gpu/media/vaapi_drm_picture.h:42: virtual bool DownloadFromSurface( drop virtual https://codereview.chromium.org/490233002/diff/620001/content/common/gpu/medi... File content/common/gpu/media/vaapi_picture.h (right): https://codereview.chromium.org/490233002/diff/620001/content/common/gpu/medi... content/common/gpu/media/vaapi_picture.h:31: virtual ~VaapiPicture() {} s/virtual/override/ https://codereview.chromium.org/490233002/diff/620001/content/common/gpu/medi... File content/common/gpu/media/vaapi_tfp_picture.h (right): https://codereview.chromium.org/490233002/diff/620001/content/common/gpu/medi... content/common/gpu/media/vaapi_tfp_picture.h:37: virtual ~VaapiTFPPicture(); s/virtual/override/ https://codereview.chromium.org/490233002/diff/620001/content/common/gpu/medi... File content/common/gpu/media/vaapi_wrapper.h (right): https://codereview.chromium.org/490233002/diff/620001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.h:43: class CONTENT_EXPORT VaapiWrapper : public base::RefCounted<VaapiWrapper> { what's necessitates ref-counting? is it possible to rework things so we don't need this?
(Replying via e-mail since the review tool is not working) content/common/sandbox_linux/bpf_gpu_policy_linux.cc lgtm since marcheu is happy with the approach. I dislike having to add yet more preload in the GPU sandbox policy, but it's fine for now. Adding jorgelo@ and leecam@ to cc: for their information. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Sorry for the delay. https://codereview.chromium.org/490233002/diff/620001/content/common/gpu/medi... File content/common/gpu/media/rendering_helper.cc (right): https://codereview.chromium.org/490233002/diff/620001/content/common/gpu/medi... content/common/gpu/media/rendering_helper.cc:80: virtual ~StubOzoneDelegate() {} On 2014/12/10 18:56:48, scherkus wrote: > IIRC this should be override and we can drop virtual from the rest (we only need > one specifier) Done. https://codereview.chromium.org/490233002/diff/620001/content/common/gpu/medi... content/common/gpu/media/rendering_helper.cc:103: gfx::AcceleratedWidget GetAcceleratedWidget() const { return widget_; } On 2014/12/10 18:56:48, scherkus wrote: > nit: for these types of accessors they should be unix_hacker() style (similar to > platform_window() below) Done. https://codereview.chromium.org/490233002/diff/620001/content/common/gpu/medi... content/common/gpu/media/rendering_helper.cc:113: }; On 2014/12/10 18:56:48, scherkus wrote: > DISALLOW etc... Done. https://codereview.chromium.org/490233002/diff/620001/content/common/gpu/medi... File content/common/gpu/media/vaapi_drm_picture.h (right): https://codereview.chromium.org/490233002/diff/620001/content/common/gpu/medi... content/common/gpu/media/vaapi_drm_picture.h:38: virtual ~VaapiDrmPicture(); On 2014/12/10 18:56:48, scherkus wrote: > s/virtual/override/ Done. https://codereview.chromium.org/490233002/diff/620001/content/common/gpu/medi... content/common/gpu/media/vaapi_drm_picture.h:42: virtual bool DownloadFromSurface( On 2014/12/10 18:56:48, scherkus wrote: > drop virtual Done. https://codereview.chromium.org/490233002/diff/620001/content/common/gpu/medi... File content/common/gpu/media/vaapi_picture.h (right): https://codereview.chromium.org/490233002/diff/620001/content/common/gpu/medi... content/common/gpu/media/vaapi_picture.h:31: virtual ~VaapiPicture() {} On 2014/12/10 18:56:48, scherkus wrote: > s/virtual/override/ Replacing virtual by override doesn't compile on ChromeOS : In file included from src/content/common/gpu/media/vaapi_picture.cc:5:0: src/content/common/gpu/media/vaapi_picture.h:31:3: error: 'content::VaapiPicture::~VaapiPicture()' marked override, but does not override ~VaapiPicture() override {} ^ In file included from src/content/common/gpu/media/vaapi_picture.cc:12:0: src/content/common/gpu/media/vaapi_drm_picture.h:38:3: error: 'content::VaapiDrmPicture::~VaapiDrmPicture()' marked override, but does not override ~VaapiDrmPicture() override; ^ In file included from src/content/common/gpu/media/vaapi_picture.h:14:0, from src/content/common/gpu/media/vaapi_picture.cc:5: src/base/memory/linked_ptr.h: In instantiation of 'void linked_ptr<T>::depart() [with T = content::VaapiPicture]': src/base/memory/linked_ptr.h:84:26: required from 'linked_ptr<T>::~linked_ptr() [with T = content::VaapiPicture]' src/content/common/gpu/media/vaapi_picture.cc:25:28: required from here src/base/memory/linked_ptr.h:146:25: error: deleting object of abstract class type 'content::VaapiPicture' which has non-virtual destructor will cause undefined behaviour [-Werror=delete-non-virtual-dtor] if (link_.depart()) delete value_; ^ cc1plus: all warnings being treated as errors https://codereview.chromium.org/490233002/diff/620001/content/common/gpu/medi... File content/common/gpu/media/vaapi_tfp_picture.h (right): https://codereview.chromium.org/490233002/diff/620001/content/common/gpu/medi... content/common/gpu/media/vaapi_tfp_picture.h:37: virtual ~VaapiTFPPicture(); On 2014/12/10 18:56:48, scherkus wrote: > s/virtual/override/ Done. https://codereview.chromium.org/490233002/diff/620001/content/common/gpu/medi... File content/common/gpu/media/vaapi_wrapper.h (right): https://codereview.chromium.org/490233002/diff/620001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.h:43: class CONTENT_EXPORT VaapiWrapper : public base::RefCounted<VaapiWrapper> { On 2014/12/10 18:56:48, scherkus wrote: > what's necessitates ref-counting? is it possible to rework things so we don't > need this? To support X11 and Ozone, we need to abstract the implementation of the output pictures. The way we convert a NV12 picture to a RGBA picture is slightly different, on X11 we use vaPutSurface(), on Ozone we use the video post processing API of LibVA. That's one reason why pictures need to have access to the VaapiWrapper. The other reason is related to the use of VASurface (va_surface.h) in VaapiDrmPicture. The destructor of VASurface is going to call into VaapiWrapper. I implemented VaapiPicture subclasses using weak pointers in a previous iteration of this CL, but Pawel objected to that.
sadrul@chromium.org changed reviewers: - sadrul@chromium.org
reveman@ : Could you lgtm the ui/gl/gl_image_glx.[ch] if you're still ok with it? vrk@ : Could you review the changes in media/? jln@ : Could you lgtm in the tool? Thanks
ui/gl/gl_image_glx.* lgtm
https://codereview.chromium.org/490233002/diff/620001/content/common/gpu/medi... File content/common/gpu/media/vaapi_wrapper.h (right): https://codereview.chromium.org/490233002/diff/620001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.h:43: class CONTENT_EXPORT VaapiWrapper : public base::RefCounted<VaapiWrapper> { On 2014/12/13 15:23:54, llandwerlin wrote: > On 2014/12/10 18:56:48, scherkus wrote: > > what's necessitates ref-counting? is it possible to rework things so we don't > > need this? > > To support X11 and Ozone, we need to abstract the implementation of the output > pictures. > The way we convert a NV12 picture to a RGBA picture is slightly different, on > X11 we use vaPutSurface(), on Ozone we use the video post processing API of > LibVA. That's one reason why pictures need to have access to the VaapiWrapper. As long as X11 needs to be supported, this is the best way in my opinion. I don't think we should expend the effort to now rework the X11 backend in libva just to satisfy this concern about refcounts. Also we have the reason below too: > The other reason is related to the use of VASurface (va_surface.h) in > VaapiDrmPicture. The destructor of VASurface is going to call into VaapiWrapper. > > I implemented VaapiPicture subclasses using weak pointers in a previous > iteration of this CL, but Pawel objected to that. Sean
lgtm
This CL is too big, I'm sorry. Can you split it into several CLs, e.g. at least: 1- the refactoring that only touches the X11 parts 2- the new code that implements the ozone parts.
https://codereview.chromium.org/490233002/diff/660001/content/common/gpu/medi... File content/common/gpu/media/vaapi_picture.cc (right): https://codereview.chromium.org/490233002/diff/660001/content/common/gpu/medi... content/common/gpu/media/vaapi_picture.cc:28: vaapi_wrapper, reinterpret_cast<gfx::GLContextGLX*>(gl_context), use static_cast instead of reinterpret_cast. Can you add a DCHECK(gfx::GetGLImplementation() != gfx::kGLImplementationDesktopGL), otherwise the cast is not valid. https://codereview.chromium.org/490233002/diff/660001/content/common/gpu/medi... File content/common/gpu/media/vaapi_tfp_picture.cc (right): https://codereview.chromium.org/490233002/diff/660001/content/common/gpu/medi... content/common/gpu/media/vaapi_tfp_picture.cc:26: x_display_(glx_context_->display()), It looks like the only reason you need the GLContextGLX is to get to the display. Maybe you can save yourself some trouble, and use gfx::GetXDisplay() directly? https://codereview.chromium.org/490233002/diff/660001/content/common/gpu/medi... File content/common/gpu/media/vaapi_wrapper.cc (right): https://codereview.chromium.org/490233002/diff/660001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.cc:62: } Doesn't look like this is needed. https://codereview.chromium.org/490233002/diff/660001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.cc:475: base::AutoLock auto_lock(va_lock_); I see you're using a lock everywhere. I take it that VaapiWrapper is called from multiple threads. Does that mean we should use RefCountedThreadSafe instead of RefCounted? https://codereview.chromium.org/490233002/diff/660001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.cc:797: if (va_vpp_buffer_id_ == VA_INVALID_ID) nit: you need {} per style guide.
https://codereview.chromium.org/490233002/diff/660001/content/common/gpu/medi... File content/common/gpu/media/vaapi_picture.cc (right): https://codereview.chromium.org/490233002/diff/660001/content/common/gpu/medi... content/common/gpu/media/vaapi_picture.cc:28: vaapi_wrapper, reinterpret_cast<gfx::GLContextGLX*>(gl_context), On 2014/12/16 20:46:43, piman (Very slow to review) wrote: > use static_cast instead of reinterpret_cast. > > Can you add a DCHECK(gfx::GetGLImplementation() != > gfx::kGLImplementationDesktopGL), otherwise the cast is not valid. Removing the gl context, this dcheck won't be needed. There is already a check in the Initialize function of VaapiVideoDecodeAccelerator. https://codereview.chromium.org/490233002/diff/660001/content/common/gpu/medi... File content/common/gpu/media/vaapi_tfp_picture.cc (right): https://codereview.chromium.org/490233002/diff/660001/content/common/gpu/medi... content/common/gpu/media/vaapi_tfp_picture.cc:26: x_display_(glx_context_->display()), On 2014/12/16 20:46:43, piman (Very slow to review) wrote: > It looks like the only reason you need the GLContextGLX is to get to the > display. Maybe you can save yourself some trouble, and use gfx::GetXDisplay() > directly? Thanks, I can remove the gl context. https://codereview.chromium.org/490233002/diff/660001/content/common/gpu/medi... File content/common/gpu/media/vaapi_wrapper.cc (right): https://codereview.chromium.org/490233002/diff/660001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.cc:62: } On 2014/12/16 20:46:43, piman (Very slow to review) wrote: > Doesn't look like this is needed. Acknowledged. https://codereview.chromium.org/490233002/diff/660001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.cc:475: base::AutoLock auto_lock(va_lock_); On 2014/12/16 20:46:43, piman (Very slow to review) wrote: > I see you're using a lock everywhere. I take it that VaapiWrapper is called from > multiple threads. Does that mean we should use RefCountedThreadSafe instead of > RefCounted? It's a good question, as far as I can tell, VaapiWrapper is not accessed from different thread. So I'm not sure why we need locks, I'm just trying to be consistent with the existing code. Maybe posciak@ can confirm the threading situation. https://codereview.chromium.org/490233002/diff/660001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.cc:797: if (va_vpp_buffer_id_ == VA_INVALID_ID) On 2014/12/16 20:46:43, piman (Very slow to review) wrote: > nit: you need {} per style guide. Acknowledged.
I think we can close this CL please? |
