Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
825163004:
Add Vaapi support on Ozone/Freon (Closed)
|
Created:
5 years, 12 months ago by llandwerlin-old Modified:
5 years, 11 months ago Reviewers:
dnicoara, sadrul, Owen Lin, piman, alexst (slow to review), spang, Pawel Osciak, scherkus (not reviewing) CC:
chromium-reviews, ozone-reviews_chromium.org, posciak+watch_chromium.org, jam, mcasas+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, kalyank, piman+watch_chromium.org, wjia+watch_chromium.org, jln+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd Vaapi support on Ozone/Freon
BUG=403058
TEST=video_decode_accelerator_unittest and video_encode_accelerator_unittest
TBR=jln,scherkus
Committed: https://crrev.com/b056abc3ccfaaa7da280084ce55724a0f07d85a0
Cr-Commit-Position: refs/heads/master@{#309863}
Patch Set 1 #
Total comments: 76
Patch Set 2 : Pawel's review #Patch Set 3 : Fix const issue #
Total comments: 23
Patch Set 4 : More nits #Patch Set 5 : Copy attributes argument #Patch Set 6 : Fix build issue #
Total comments: 1
Patch Set 7 : Fix comment in VaapiWrapper #Messages
Total messages: 42 (12 generated)
lionel.g.landwerlin@intel.com changed reviewers: + alexst@chromium.org, piman@chromium.org, posciak@chromium.org, scherkus@chromium.org
alexst@ posciak@ scherkus@ piman@, this is a follow up on https://codereview.chromium.org/817023005/ to enable vaapi on freon. If possible, please run the ozone try bots, Thanks.
lionel.g.landwerlin@intel.com changed reviewers: - alexst@chromium.org
lionel.g.landwerlin@intel.com changed reviewers: + dnicoara@chromium.org
Just realized alexst@ is ooo.
https://codereview.chromium.org/825163004/diff/1/content/common/gpu/media/ren... File content/common/gpu/media/rendering_helper.cc (right): https://codereview.chromium.org/825163004/diff/1/content/common/gpu/media/ren... content/common/gpu/media/rendering_helper.cc:382: // On Ozone the VSyncProvider can't provide a vsync_internal until s/vsync_internal/vsync interval/ https://codereview.chromium.org/825163004/diff/1/content/common/gpu/media/va_... File content/common/gpu/media/va_stub_header.fragment (right): https://codereview.chromium.org/825163004/diff/1/content/common/gpu/media/va_... content/common/gpu/media/va_stub_header.fragment:8: #include "third_party/libva/va/va_drmcommon.h" Do we need this header at all in this CL? https://codereview.chromium.org/825163004/diff/1/content/common/gpu/media/vaa... File content/common/gpu/media/vaapi_drm_picture.cc (right): https://codereview.chromium.org/825163004/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_drm_picture.cc:11: #include "third_party/libva/va/va_drmcommon.h" Not needed perhaps? https://codereview.chromium.org/825163004/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_drm_picture.cc:24: const base::Callback<bool(void)> make_context_current, reference please. https://codereview.chromium.org/825163004/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_drm_picture.cc:34: if (egl_image_.get() && make_context_current_.Run()) { get() not needed anymore. https://codereview.chromium.org/825163004/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_drm_picture.cc:42: bool VaapiDrmPicture::Initialize() { Please add comments what is happening in this method and why. https://codereview.chromium.org/825163004/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_drm_picture.cc:44: VASurfaceAttribExternalBuffers va_attrib_extbuf; Please define local variables immediately before use. https://codereview.chromium.org/825163004/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_drm_picture.cc:46: ui::OzonePlatform* platform = ui::OzonePlatform::GetInstance(); Please check the return value. https://codereview.chromium.org/825163004/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_drm_picture.cc:47: ui::SurfaceFactoryOzone* factory = platform->GetSurfaceFactoryOzone(); Ditto. https://codereview.chromium.org/825163004/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_drm_picture.cc:52: if (!pixmap_.get()) { get() not needed anymore. https://codereview.chromium.org/825163004/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_drm_picture.cc:59: LOG(ERROR) << "Failed get dmabuf from an Ozone NativePixmap"; s/get/to get/ https://codereview.chromium.org/825163004/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_drm_picture.cc:89: if (!va_surface_.get()) { get() not needed anymore. https://codereview.chromium.org/825163004/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_drm_picture.cc:98: if (!egl_image_->Initialize(base::FileDescriptor(dmabuf_fd, false), Why not autoclose? https://codereview.chromium.org/825163004/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_drm_picture.cc:100: LOG(ERROR) << "Failed to create an EGLImage for an Ozone NativePixmap"; s/EGLImage/GLImageLinuxDMABuffer/ https://codereview.chromium.org/825163004/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_drm_picture.cc:107: LOG(ERROR) << "Failed to bind texture to EGLImage"; s/EGLImage/GLImage/ https://codereview.chromium.org/825163004/diff/1/content/common/gpu/media/vaa... File content/common/gpu/media/vaapi_drm_picture.h (right): https://codereview.chromium.org/825163004/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_drm_picture.h:45: VaapiWrapper *vaapi_wrapper_; // Not owned. '*' next to class name please. https://codereview.chromium.org/825163004/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_drm_picture.h:47: scoped_refptr<VASurface> va_surface_; Do we need scopers in this class to be refcounted? https://codereview.chromium.org/825163004/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_drm_picture.h:48: scoped_refptr<ui::NativePixmap> pixmap_; Please add a comment what this is. https://codereview.chromium.org/825163004/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_drm_picture.h:49: scoped_refptr<gfx::GLImageLinuxDMABuffer> egl_image_; s/egl_image/gl_image/ https://codereview.chromium.org/825163004/diff/1/content/common/gpu/media/vaa... File content/common/gpu/media/vaapi_video_decode_accelerator.cc (right): https://codereview.chromium.org/825163004/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_video_decode_accelerator.cc:117: "EGLGLES2."; s/ "EGLGLES2.";/<< "EGLGLES2.";/ https://codereview.chromium.org/825163004/diff/1/content/common/gpu/media/vaa... File content/common/gpu/media/vaapi_wrapper.cc (right): https://codereview.chromium.org/825163004/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_wrapper.cc:23: #include "third_party/libva/va/va_drmcommon.h" Needed? https://codereview.chromium.org/825163004/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_wrapper.cc:242: ui::OzonePlatform* platform = ui::OzonePlatform::GetInstance(); Please check return values. https://codereview.chromium.org/825163004/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_wrapper.cc:471: scoped_refptr<VASurface> va_surface; Please define variables at use site. https://codereview.chromium.org/825163004/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_wrapper.cc:481: make_scoped_refptr(this))); We can't do this. We can't have VaapiVDA keep a scoped_ptr and then some other classes scoped_refptrs. We can either maintain the lifetime of the wrapper manually via scoped_ptr and unowned pointers, or via refptrs, but we can't mix these. https://codereview.chromium.org/825163004/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_wrapper.cc:490: VA_LOG_ON_ERROR(va_res, "vaDestroySurfaces on output surface failed"); s/output// https://codereview.chromium.org/825163004/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_wrapper.cc:812: if (va_vpp_buffer_id_ == VA_INVALID_ID) { Maybe just do this in Initialize() ? https://codereview.chromium.org/825163004/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_wrapper.cc:837: pipeline_param->output_background_color = 0xff000000; Do we have a constant we could use here? https://codereview.chromium.org/825163004/diff/1/content/common/gpu/media/vaa... File content/common/gpu/media/vaapi_wrapper.h (right): https://codereview.chromium.org/825163004/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_wrapper.h:77: // the surface is transfered to the caller. It differs from surfaces s/transfered/transferred/ https://codereview.chromium.org/825163004/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_wrapper.h:78: // created using CreateSurfaces() where VaapiWrapper is the owner of s/()/(),/ https://codereview.chromium.org/825163004/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_wrapper.h:80: scoped_refptr<VASurface> CreateUnownedSurface(unsigned int va_format, Should we also be passing va_format to CreateSurfaces? https://codereview.chromium.org/825163004/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_wrapper.h:82: VASurfaceAttrib* va_attribs, Could we instead pass a vector, remove num_va_attribs and then use &va_attribs[0] please? https://codereview.chromium.org/825163004/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_wrapper.h:178: void DestroyUnownedSurface(VASurfaceID va_surface); s/va_surface/va_surface_id/ https://codereview.chromium.org/825163004/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_wrapper.h:231: // VPP context Please describe what VPP is and what it is used for. https://codereview.chromium.org/825163004/diff/1/content/common/gpu/media/vid... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/825163004/diff/1/content/common/gpu/media/vid... content/common/gpu/media/video_encode_accelerator_unittest.cc:1295: ui::OzonePlatform::InitializeForUI(); Do we need these in encoder test if we don't display anything? https://codereview.chromium.org/825163004/diff/1/content/common/sandbox_linux... File content/common/sandbox_linux/bpf_gpu_policy_linux.cc (right): https://codereview.chromium.org/825163004/diff/1/content/common/sandbox_linux... content/common/sandbox_linux/bpf_gpu_policy_linux.cc:277: dlopen("libva-drm.so.1", RTLD_NOW | RTLD_GLOBAL | RTLD_NODELETE); Please keep lexicographical order. We also want to have ifdefined (USE_OZONE) etc. https://codereview.chromium.org/825163004/diff/1/media/test/data/test-25fps.h... File media/test/data/test-25fps.h264.md5 (right): https://codereview.chromium.org/825163004/diff/1/media/test/data/test-25fps.h... media/test/data/test-25fps.h264.md5:2: # Intel Haswell - EGL Interesting. So HSW and IVB are different on EGL, but not on X11?
posciak@chromium.org changed reviewers: + spang@chromium.org
+spang@ PTAL, since alexst@ is ooo
posciak@chromium.org changed reviewers: + owenlin@chromium.org
+owenlin@ for VDA tests and rendering helper please.
https://codereview.chromium.org/825163004/diff/1/content/common/gpu/media/ren... File content/common/gpu/media/rendering_helper.cc (right): https://codereview.chromium.org/825163004/diff/1/content/common/gpu/media/ren... content/common/gpu/media/rendering_helper.cc:382: // On Ozone the VSyncProvider can't provide a vsync_internal until On 2014/12/26 00:38:55, Pawel Osciak wrote: > s/vsync_internal/vsync interval/ Done. https://codereview.chromium.org/825163004/diff/1/content/common/gpu/media/va_... File content/common/gpu/media/va_stub_header.fragment (right): https://codereview.chromium.org/825163004/diff/1/content/common/gpu/media/va_... content/common/gpu/media/va_stub_header.fragment:8: #include "third_party/libva/va/va_drmcommon.h" On 2014/12/26 00:38:55, Pawel Osciak wrote: > Do we need this header at all in this CL? Maybe I can remove it from this template file, but we need a define from it. https://codereview.chromium.org/825163004/diff/1/content/common/gpu/media/vaa... File content/common/gpu/media/vaapi_drm_picture.cc (right): https://codereview.chromium.org/825163004/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_drm_picture.cc:11: #include "third_party/libva/va/va_drmcommon.h" On 2014/12/26 00:38:55, Pawel Osciak wrote: > Not needed perhaps? Needed for VA_SURFACE_ATTRIB_MEM_TYPE_DRM_PRIME. https://codereview.chromium.org/825163004/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_drm_picture.cc:24: const base::Callback<bool(void)> make_context_current, On 2014/12/26 00:38:55, Pawel Osciak wrote: > reference please. Done. https://codereview.chromium.org/825163004/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_drm_picture.cc:34: if (egl_image_.get() && make_context_current_.Run()) { On 2014/12/26 00:38:55, Pawel Osciak wrote: > get() not needed anymore. Done. https://codereview.chromium.org/825163004/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_drm_picture.cc:42: bool VaapiDrmPicture::Initialize() { On 2014/12/26 00:38:55, Pawel Osciak wrote: > Please add comments what is happening in this method and why. Acknowledged. https://codereview.chromium.org/825163004/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_drm_picture.cc:44: VASurfaceAttribExternalBuffers va_attrib_extbuf; On 2014/12/26 00:38:55, Pawel Osciak wrote: > Please define local variables immediately before use. Done. https://codereview.chromium.org/825163004/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_drm_picture.cc:46: ui::OzonePlatform* platform = ui::OzonePlatform::GetInstance(); On 2014/12/26 00:38:55, Pawel Osciak wrote: > Please check the return value. I think we went through this before : https://codereview.chromium.org/490233002/diff/540001/content/common/gpu/medi... But if you want checks, I will add them. https://codereview.chromium.org/825163004/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_drm_picture.cc:47: ui::SurfaceFactoryOzone* factory = platform->GetSurfaceFactoryOzone(); On 2014/12/26 00:38:55, Pawel Osciak wrote: > Ditto. Same. https://codereview.chromium.org/825163004/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_drm_picture.cc:52: if (!pixmap_.get()) { On 2014/12/26 00:38:55, Pawel Osciak wrote: > get() not needed anymore. Done. https://codereview.chromium.org/825163004/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_drm_picture.cc:59: LOG(ERROR) << "Failed get dmabuf from an Ozone NativePixmap"; On 2014/12/26 00:38:55, Pawel Osciak wrote: > s/get/to get/ Done. https://codereview.chromium.org/825163004/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_drm_picture.cc:89: if (!va_surface_.get()) { On 2014/12/26 00:38:55, Pawel Osciak wrote: > get() not needed anymore. Done. https://codereview.chromium.org/825163004/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_drm_picture.cc:98: if (!egl_image_->Initialize(base::FileDescriptor(dmabuf_fd, false), On 2014/12/26 00:38:55, Pawel Osciak wrote: > Why not autoclose? GbmPixmap does this for us : https://code.google.com/p/chromium/codesearch#chromium/src/ui/ozone/platform/... https://codereview.chromium.org/825163004/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_drm_picture.cc:100: LOG(ERROR) << "Failed to create an EGLImage for an Ozone NativePixmap"; On 2014/12/26 00:38:55, Pawel Osciak wrote: > s/EGLImage/GLImageLinuxDMABuffer/ Done. https://codereview.chromium.org/825163004/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_drm_picture.cc:107: LOG(ERROR) << "Failed to bind texture to EGLImage"; On 2014/12/26 00:38:55, Pawel Osciak wrote: > s/EGLImage/GLImage/ Done. https://codereview.chromium.org/825163004/diff/1/content/common/gpu/media/vaa... File content/common/gpu/media/vaapi_drm_picture.h (right): https://codereview.chromium.org/825163004/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_drm_picture.h:45: VaapiWrapper *vaapi_wrapper_; // Not owned. On 2014/12/26 00:38:56, Pawel Osciak wrote: > '*' next to class name please. Done. https://codereview.chromium.org/825163004/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_drm_picture.h:47: scoped_refptr<VASurface> va_surface_; On 2014/12/26 00:38:55, Pawel Osciak wrote: > Do we need scopers in this class to be refcounted? I can probably move them to scoped_ptr. This is going to bring back the .get() in vaapi_drm_picture.cc though. https://codereview.chromium.org/825163004/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_drm_picture.h:48: scoped_refptr<ui::NativePixmap> pixmap_; On 2014/12/26 00:38:55, Pawel Osciak wrote: > Please add a comment what this is. Done. https://codereview.chromium.org/825163004/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_drm_picture.h:49: scoped_refptr<gfx::GLImageLinuxDMABuffer> egl_image_; On 2014/12/26 00:38:56, Pawel Osciak wrote: > s/egl_image/gl_image/ Done. https://codereview.chromium.org/825163004/diff/1/content/common/gpu/media/vaa... File content/common/gpu/media/vaapi_video_decode_accelerator.cc (right): https://codereview.chromium.org/825163004/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_video_decode_accelerator.cc:117: "EGLGLES2."; On 2014/12/26 00:38:56, Pawel Osciak wrote: > s/ "EGLGLES2.";/<< "EGLGLES2.";/ Done. https://codereview.chromium.org/825163004/diff/1/content/common/gpu/media/vaa... File content/common/gpu/media/vaapi_wrapper.cc (right): https://codereview.chromium.org/825163004/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_wrapper.cc:23: #include "third_party/libva/va/va_drmcommon.h" On 2014/12/26 00:38:56, Pawel Osciak wrote: > Needed? I can probably remove from here. https://codereview.chromium.org/825163004/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_wrapper.cc:242: ui::OzonePlatform* platform = ui::OzonePlatform::GetInstance(); On 2014/12/26 00:38:56, Pawel Osciak wrote: > Please check return values. Same as https://codereview.chromium.org/490233002/diff/540001/content/common/gpu/medi... I can add checks, but I would argue it's about the same as checking the value of gfx::GetXDisplay(). https://codereview.chromium.org/825163004/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_wrapper.cc:471: scoped_refptr<VASurface> va_surface; On 2014/12/26 00:38:56, Pawel Osciak wrote: > Please define variables at use site. Done. https://codereview.chromium.org/825163004/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_wrapper.cc:481: make_scoped_refptr(this))); On 2014/12/26 00:38:56, Pawel Osciak wrote: > We can't do this. We can't have VaapiVDA keep a scoped_ptr and then some other > classes scoped_refptrs. We can either maintain the lifetime of the wrapper > manually via scoped_ptr and unowned pointers, or via refptrs, but we can't mix > these. Thanks, will change to Unretained. https://codereview.chromium.org/825163004/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_wrapper.cc:490: VA_LOG_ON_ERROR(va_res, "vaDestroySurfaces on output surface failed"); On 2014/12/26 00:38:56, Pawel Osciak wrote: > s/output// Done. https://codereview.chromium.org/825163004/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_wrapper.cc:812: if (va_vpp_buffer_id_ == VA_INVALID_ID) { On 2014/12/26 00:38:56, Pawel Osciak wrote: > Maybe just do this in Initialize() ? That's up to you. The reason it's done lazily at the moment is to avoid to do it for encoders instances or on X11. https://codereview.chromium.org/825163004/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_wrapper.cc:837: pipeline_param->output_background_color = 0xff000000; Not yet. I can add one here, let me know. https://codereview.chromium.org/825163004/diff/1/content/common/gpu/media/vaa... File content/common/gpu/media/vaapi_wrapper.h (right): https://codereview.chromium.org/825163004/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_wrapper.h:77: // the surface is transfered to the caller. It differs from surfaces On 2014/12/26 00:38:56, Pawel Osciak wrote: > s/transfered/transferred/ Done. https://codereview.chromium.org/825163004/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_wrapper.h:78: // created using CreateSurfaces() where VaapiWrapper is the owner of On 2014/12/26 00:38:56, Pawel Osciak wrote: > s/()/(),/ Done. https://codereview.chromium.org/825163004/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_wrapper.h:80: scoped_refptr<VASurface> CreateUnownedSurface(unsigned int va_format, On 2014/12/26 00:38:56, Pawel Osciak wrote: > Should we also be passing va_format to CreateSurfaces? Surfaces used by the decoder/encoder are assumed to be of NV12 format. It seems these assumptions are in VaapiWrapper at the moment. Adding va_format to CreateSurfaces() moves the assumption from VaapiWrapper to VaapiV[DE]A. Maybe we can keep that for another CL? https://codereview.chromium.org/825163004/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_wrapper.h:82: VASurfaceAttrib* va_attribs, On 2014/12/26 00:38:56, Pawel Osciak wrote: > Could we instead pass a vector, remove num_va_attribs and then use > &va_attribs[0] please? Done. https://codereview.chromium.org/825163004/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_wrapper.h:178: void DestroyUnownedSurface(VASurfaceID va_surface); On 2014/12/26 00:38:56, Pawel Osciak wrote: > s/va_surface/va_surface_id/ Done. https://codereview.chromium.org/825163004/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_wrapper.h:231: // VPP context On 2014/12/26 00:38:56, Pawel Osciak wrote: > Please describe what VPP is and what it is used for. Done. https://codereview.chromium.org/825163004/diff/1/content/common/gpu/media/vid... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/825163004/diff/1/content/common/gpu/media/vid... content/common/gpu/media/video_encode_accelerator_unittest.cc:1295: ui::OzonePlatform::InitializeForUI(); On 2014/12/26 00:38:56, Pawel Osciak wrote: > Do we need these in encoder test if we don't display anything? Yes, because we need access to the drm fd, initialized by GPU process code, to initialize vaapi. https://codereview.chromium.org/825163004/diff/1/content/common/sandbox_linux... File content/common/sandbox_linux/bpf_gpu_policy_linux.cc (right): https://codereview.chromium.org/825163004/diff/1/content/common/sandbox_linux... content/common/sandbox_linux/bpf_gpu_policy_linux.cc:277: dlopen("libva-drm.so.1", RTLD_NOW | RTLD_GLOBAL | RTLD_NODELETE); On 2014/12/26 00:38:56, Pawel Osciak wrote: > Please keep lexicographical order. We also want to have ifdefined (USE_OZONE) > etc. Done. https://codereview.chromium.org/825163004/diff/1/media/test/data/test-25fps.h... File media/test/data/test-25fps.h264.md5 (right): https://codereview.chromium.org/825163004/diff/1/media/test/data/test-25fps.h... media/test/data/test-25fps.h264.md5:2: # Intel Haswell - EGL On 2014/12/26 00:38:56, Pawel Osciak wrote: > Interesting. So HSW and IVB are different on EGL, but not on X11? Yes, interesting. The intel driver for LibVA appears to have 2 implementations converting/scaling pictures. That's the path on X11 (with vaPutSurface) : http://cgit.freedesktop.org/vaapi/intel-driver/tree/src/i965_render.c That's the path on Ozone (with VPP API) : http://cgit.freedesktop.org/vaapi/intel-driver/tree/src/i965_drv_video.c I'm not exactly sure what's wrong with the later so that it generates differences between HSW and IVB, but I've noticed that it has hardcoded colorimetry coefficients (BT709 if I recall correctly), so there are definitely problems there.
https://chromiumcodereview.appspot.com/825163004/diff/1/content/common/gpu/me... File content/common/gpu/media/vaapi_drm_picture.cc (right): https://chromiumcodereview.appspot.com/825163004/diff/1/content/common/gpu/me... content/common/gpu/media/vaapi_drm_picture.cc:11: #include "third_party/libva/va/va_drmcommon.h" On 2014/12/26 02:50:01, llandwerlin wrote: > On 2014/12/26 00:38:55, Pawel Osciak wrote: > > Not needed perhaps? > > Needed for VA_SURFACE_ATTRIB_MEM_TYPE_DRM_PRIME. Right, missed that, sorry. https://chromiumcodereview.appspot.com/825163004/diff/1/content/common/gpu/me... content/common/gpu/media/vaapi_drm_picture.cc:46: ui::OzonePlatform* platform = ui::OzonePlatform::GetInstance(); On 2014/12/26 02:50:01, llandwerlin wrote: > On 2014/12/26 00:38:55, Pawel Osciak wrote: > > Please check the return value. > > I think we went through this before : > > https://codereview.chromium.org/490233002/diff/540001/content/common/gpu/medi... > > But if you want checks, I will add them. Yes please. Sure something went really wrong, but we still don't want to randomly crash the process on user if we can help it. https://chromiumcodereview.appspot.com/825163004/diff/1/content/common/gpu/me... content/common/gpu/media/vaapi_drm_picture.cc:98: if (!egl_image_->Initialize(base::FileDescriptor(dmabuf_fd, false), On 2014/12/26 02:50:01, llandwerlin wrote: > On 2014/12/26 00:38:55, Pawel Osciak wrote: > > Why not autoclose? > > GbmPixmap does this for us : > https://code.google.com/p/chromium/codesearch#chromium/src/ui/ozone/platform/... Acknowledged. https://chromiumcodereview.appspot.com/825163004/diff/1/content/common/gpu/me... File content/common/gpu/media/vaapi_wrapper.cc (right): https://chromiumcodereview.appspot.com/825163004/diff/1/content/common/gpu/me... content/common/gpu/media/vaapi_wrapper.cc:812: if (va_vpp_buffer_id_ == VA_INVALID_ID) { On 2014/12/26 02:50:02, llandwerlin wrote: > On 2014/12/26 00:38:56, Pawel Osciak wrote: > > Maybe just do this in Initialize() ? > > That's up to you. The reason it's done lazily at the moment is to avoid to do it > for encoders instances or on X11. Acknowledged. https://chromiumcodereview.appspot.com/825163004/diff/40001/content/common/gp... File content/common/gpu/media/rendering_helper.cc (right): https://chromiumcodereview.appspot.com/825163004/diff/40001/content/common/gp... content/common/gpu/media/rendering_helper.cc:382: // On Ozone the VSyncProvider can't provide a vsync internal until Still says "internal" :) https://chromiumcodereview.appspot.com/825163004/diff/40001/content/common/gp... File content/common/gpu/media/vaapi_drm_picture.cc (right): https://chromiumcodereview.appspot.com/825163004/diff/40001/content/common/gp... content/common/gpu/media/vaapi_drm_picture.cc:44: // memory buffer, so we can output decoded pictures to it using Is this really what's happening? Is the VASurface backed by the same memory buffer? They are different formats for one thing, and then I don't think the blit in DownloadSurface is in place... https://chromiumcodereview.appspot.com/825163004/diff/40001/content/common/gp... File content/common/gpu/media/vaapi_drm_picture.h (right): https://chromiumcodereview.appspot.com/825163004/diff/40001/content/common/gp... content/common/gpu/media/vaapi_drm_picture.h:48: // Ozone buffer, the storage of the EGLImage and the VASurface. I don't think the pixmap is also the storage of va surface? Would we have to download to eglimage from the surface had that been the case? https://chromiumcodereview.appspot.com/825163004/diff/40001/content/common/gp... content/common/gpu/media/vaapi_drm_picture.h:49: scoped_refptr<ui::NativePixmap> pixmap_; Did scoped_ptrs not work out? https://chromiumcodereview.appspot.com/825163004/diff/40001/content/common/gp... content/common/gpu/media/vaapi_drm_picture.h:54: // VASurface used to transfer from NV12 pixel format. I don't think we explicitly use nv12 (or any format) for the surface anywhere in this class (even if we do so in the accelerators), do we need to say it here? My point is, if accelerators change in the future, I think this could be used with other formats assuming vaapi understood them? https://chromiumcodereview.appspot.com/825163004/diff/40001/content/common/gp... File content/common/gpu/media/vaapi_wrapper.cc (right): https://chromiumcodereview.appspot.com/825163004/diff/40001/content/common/gp... content/common/gpu/media/vaapi_wrapper.cc:466: std::vector<VASurfaceAttrib>& va_attribs) { Please const with references. https://chromiumcodereview.appspot.com/825163004/diff/40001/content/common/gp... content/common/gpu/media/vaapi_wrapper.cc:474: scoped_refptr<VASurface> va_surface; This should go after VA_SUCCESS_OR_RETURN. https://chromiumcodereview.appspot.com/825163004/diff/40001/content/common/gp... content/common/gpu/media/vaapi_wrapper.cc:480: base::Bind(&VaapiWrapper::DestroyUnownedSurface, base::Unretained(this))); Please document why Unretained is safe. https://chromiumcodereview.appspot.com/825163004/diff/40001/content/common/gp... File content/common/gpu/media/vaapi_wrapper.h (right): https://chromiumcodereview.appspot.com/825163004/diff/40001/content/common/gp... content/common/gpu/media/vaapi_wrapper.h:83: std::vector<VASurfaceAttrib>& va_attribs); Please always const with references. https://chromiumcodereview.appspot.com/825163004/diff/40001/content/common/gp... content/common/gpu/media/vaapi_wrapper.h:231: // VPP (Video Post Processing) context, this is used to convert NV12 s/NV12//
https://codereview.chromium.org/825163004/diff/40001/content/common/gpu/media... File content/common/gpu/media/rendering_helper.cc (right): https://codereview.chromium.org/825163004/diff/40001/content/common/gpu/media... content/common/gpu/media/rendering_helper.cc:382: // On Ozone the VSyncProvider can't provide a vsync internal until On 2014/12/29 00:08:00, Pawel Osciak wrote: > Still says "internal" :) Ok, got it :) https://codereview.chromium.org/825163004/diff/40001/content/common/gpu/media... File content/common/gpu/media/vaapi_drm_picture.cc (right): https://codereview.chromium.org/825163004/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_drm_picture.cc:44: // memory buffer, so we can output decoded pictures to it using On 2014/12/29 00:08:00, Pawel Osciak wrote: > Is this really what's happening? Is the VASurface backed by the same memory > buffer? They are different formats for one thing, and then I don't think the > blit in DownloadSurface is in place... Maybe I didn't explain this correctly. The VDA has 2 sets of surfaces, the NV12 format ones (created using VaapiWrapper::CreateSurfaces) and the RGBA surfaces (created here). Once the decoder hardware accelerator is done working on a NV12 surface, it is then converted to a RGBA one using VaapiPicture::DownloadFromSurface. On Ozone we're going to use the Video Post Processing API of LibVA to do that conversion. That requires an input surface and an output surface. The surface we're creating here (this output surface) is backed by the pixmap allocated with Ozone. This pixmap also backs the EGLImage which is then bound to the GL texture used by the VDA client. https://codereview.chromium.org/825163004/diff/40001/content/common/gpu/media... File content/common/gpu/media/vaapi_drm_picture.h (right): https://codereview.chromium.org/825163004/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_drm_picture.h:48: // Ozone buffer, the storage of the EGLImage and the VASurface. On 2014/12/29 00:08:01, Pawel Osciak wrote: > I don't think the pixmap is also the storage of va surface? Would we have to > download to eglimage from the surface had that been the case? The pixmap is the storage of both the VASurface and the EGLImage below. https://codereview.chromium.org/825163004/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_drm_picture.h:49: scoped_refptr<ui::NativePixmap> pixmap_; On 2014/12/29 00:08:01, Pawel Osciak wrote: > Did scoped_ptrs not work out? No, the classes implementing base::RefCounted have to make their destructor protected or private, which makes it impossible to use with scoped_ptr. https://codereview.chromium.org/825163004/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_drm_picture.h:54: // VASurface used to transfer from NV12 pixel format. On 2014/12/29 00:08:01, Pawel Osciak wrote: > I don't think we explicitly use nv12 (or any format) for the surface anywhere in > this class (even if we do so in the accelerators), do we need to say it here? My > point is, if accelerators change in the future, I think this could be used with > other formats assuming vaapi understood them? Ok, I won't make reference to the input format here. https://codereview.chromium.org/825163004/diff/40001/content/common/gpu/media... File content/common/gpu/media/vaapi_wrapper.cc (right): https://codereview.chromium.org/825163004/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.cc:466: std::vector<VASurfaceAttrib>& va_attribs) { On 2014/12/29 00:08:01, Pawel Osciak wrote: > Please const with references. Couldn't get that to work because vaCreateSurfaces() doesn't take a const argument for attribs. https://codereview.chromium.org/825163004/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.cc:474: scoped_refptr<VASurface> va_surface; On 2014/12/29 00:08:01, Pawel Osciak wrote: > This should go after VA_SUCCESS_OR_RETURN. This is used by VA_SUCCESS_OR_RETURN. https://codereview.chromium.org/825163004/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.cc:480: base::Bind(&VaapiWrapper::DestroyUnownedSurface, base::Unretained(this))); On 2014/12/29 00:08:01, Pawel Osciak wrote: > Please document why Unretained is safe. Done. https://codereview.chromium.org/825163004/diff/40001/content/common/gpu/media... File content/common/gpu/media/vaapi_wrapper.h (right): https://codereview.chromium.org/825163004/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.h:231: // VPP (Video Post Processing) context, this is used to convert NV12 On 2014/12/29 00:08:01, Pawel Osciak wrote: > s/NV12// Done.
https://codereview.chromium.org/825163004/diff/40001/content/common/gpu/media... File content/common/gpu/media/vaapi_drm_picture.cc (right): https://codereview.chromium.org/825163004/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_drm_picture.cc:44: // memory buffer, so we can output decoded pictures to it using On 2014/12/29 02:30:34, llandwerlin wrote: > On 2014/12/29 00:08:00, Pawel Osciak wrote: > > Is this really what's happening? Is the VASurface backed by the same memory > > buffer? They are different formats for one thing, and then I don't think the > > blit in DownloadSurface is in place... > > Maybe I didn't explain this correctly. > The VDA has 2 sets of surfaces, the NV12 format ones (created using > VaapiWrapper::CreateSurfaces) and the RGBA surfaces (created here). > Once the decoder hardware accelerator is done working on a NV12 surface, it is > then converted to a RGBA one using VaapiPicture::DownloadFromSurface. On Ozone > we're going to use the Video Post Processing API of LibVA to do that conversion. > That requires an input surface and an output surface. > The surface we're creating here (this output surface) is backed by the pixmap > allocated with Ozone. This pixmap also backs the EGLImage which is then bound to > the GL texture used by the VDA client. Of course, sorry, thanks, otherwise we'd have had an additional copy. I'm not sure what I was thinking when I asked this, somehow I went into the x11 picture paradigm for a moment there... https://codereview.chromium.org/825163004/diff/40001/content/common/gpu/media... File content/common/gpu/media/vaapi_wrapper.cc (right): https://codereview.chromium.org/825163004/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.cc:466: std::vector<VASurfaceAttrib>& va_attribs) { On 2014/12/29 02:30:34, llandwerlin wrote: > On 2014/12/29 00:08:01, Pawel Osciak wrote: > > Please const with references. > > Couldn't get that to work because vaCreateSurfaces() doesn't take a const > argument for attribs. Let's do a copy then please. All reference arguments must be const. https://codereview.chromium.org/825163004/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.cc:474: scoped_refptr<VASurface> va_surface; On 2014/12/29 02:30:34, llandwerlin wrote: > On 2014/12/29 00:08:01, Pawel Osciak wrote: > > This should go after VA_SUCCESS_OR_RETURN. > > This is used by VA_SUCCESS_OR_RETURN. Ah funny, I didn't even notice that, since obviously it shouldn't. VA_SUCCESS_OR_RETURN should instead be on va_res from line above.
posciak@chromium.org changed reviewers: + alexst@chromium.org
https://chromiumcodereview.appspot.com/825163004/diff/40001/content/common/gp... File content/common/gpu/media/vaapi_wrapper.cc (right): https://chromiumcodereview.appspot.com/825163004/diff/40001/content/common/gp... content/common/gpu/media/vaapi_wrapper.cc:474: scoped_refptr<VASurface> va_surface; On 2014/12/29 10:41:51, Pawel Osciak wrote: > On 2014/12/29 02:30:34, llandwerlin wrote: > > On 2014/12/29 00:08:01, Pawel Osciak wrote: > > > This should go after VA_SUCCESS_OR_RETURN. > > > > This is used by VA_SUCCESS_OR_RETURN. > > Ah funny, I didn't even notice that, since obviously it shouldn't. > VA_SUCCESS_OR_RETURN should instead be on va_res from line above. My... are my ears red now. Of course this is totally correct and I'm very embarrassingly wrong. I'm really sorry!
On 2014/12/29 10:53:30, Pawel Osciak wrote: > https://chromiumcodereview.appspot.com/825163004/diff/40001/content/common/gp... > File content/common/gpu/media/vaapi_wrapper.cc (right): > > https://chromiumcodereview.appspot.com/825163004/diff/40001/content/common/gp... > content/common/gpu/media/vaapi_wrapper.cc:474: scoped_refptr<VASurface> > va_surface; > On 2014/12/29 10:41:51, Pawel Osciak wrote: > > On 2014/12/29 02:30:34, llandwerlin wrote: > > > On 2014/12/29 00:08:01, Pawel Osciak wrote: > > > > This should go after VA_SUCCESS_OR_RETURN. > > > > > > This is used by VA_SUCCESS_OR_RETURN. > > > > Ah funny, I didn't even notice that, since obviously it shouldn't. > > VA_SUCCESS_OR_RETURN should instead be on va_res from line above. > > My... are my ears red now. Of course this is totally correct and I'm very > embarrassingly wrong. I'm really sorry! No worries!
ui/ozone/ lgtm
But looks like linux_chromium_chromeos_ozone_dbg is unhappy, I think there is a missing dep there somewhere.
On 2014/12/29 12:05:05, llandwerlin wrote: > On 2014/12/29 10:53:30, Pawel Osciak wrote: > > > https://chromiumcodereview.appspot.com/825163004/diff/40001/content/common/gp... > > File content/common/gpu/media/vaapi_wrapper.cc (right): > > > > > https://chromiumcodereview.appspot.com/825163004/diff/40001/content/common/gp... > > content/common/gpu/media/vaapi_wrapper.cc:474: scoped_refptr<VASurface> > > va_surface; > > On 2014/12/29 10:41:51, Pawel Osciak wrote: > > > On 2014/12/29 02:30:34, llandwerlin wrote: > > > > On 2014/12/29 00:08:01, Pawel Osciak wrote: > > > > > This should go after VA_SUCCESS_OR_RETURN. > > > > > > > > This is used by VA_SUCCESS_OR_RETURN. > > > > > > Ah funny, I didn't even notice that, since obviously it shouldn't. > > > VA_SUCCESS_OR_RETURN should instead be on va_res from line above. > > > > My... are my ears red now. Of course this is totally correct and I'm very > > embarrassingly wrong. I'm really sorry! > > No worries! Thanks! I think we only have the const issue remaining.
On 2014/12/30 05:12:59, Pawel Osciak wrote: > On 2014/12/29 12:05:05, llandwerlin wrote: > > On 2014/12/29 10:53:30, Pawel Osciak wrote: > > > > > > https://chromiumcodereview.appspot.com/825163004/diff/40001/content/common/gp... > > > File content/common/gpu/media/vaapi_wrapper.cc (right): > > > > > > > > > https://chromiumcodereview.appspot.com/825163004/diff/40001/content/common/gp... > > > content/common/gpu/media/vaapi_wrapper.cc:474: scoped_refptr<VASurface> > > > va_surface; > > > On 2014/12/29 10:41:51, Pawel Osciak wrote: > > > > On 2014/12/29 02:30:34, llandwerlin wrote: > > > > > On 2014/12/29 00:08:01, Pawel Osciak wrote: > > > > > > This should go after VA_SUCCESS_OR_RETURN. > > > > > > > > > > This is used by VA_SUCCESS_OR_RETURN. > > > > > > > > Ah funny, I didn't even notice that, since obviously it shouldn't. > > > > VA_SUCCESS_OR_RETURN should instead be on va_res from line above. > > > > > > My... are my ears red now. Of course this is totally correct and I'm very > > > embarrassingly wrong. I'm really sorry! > > > > No worries! > > Thanks! I think we only have the const issue remaining. (And of course the build issue Alex mentioned above)
On 2014/12/30 05:14:08, Pawel Osciak wrote: > On 2014/12/30 05:12:59, Pawel Osciak wrote: > > On 2014/12/29 12:05:05, llandwerlin wrote: > > > On 2014/12/29 10:53:30, Pawel Osciak wrote: > > > > > > > > > > https://chromiumcodereview.appspot.com/825163004/diff/40001/content/common/gp... > > > > File content/common/gpu/media/vaapi_wrapper.cc (right): > > > > > > > > > > > > > > https://chromiumcodereview.appspot.com/825163004/diff/40001/content/common/gp... > > > > content/common/gpu/media/vaapi_wrapper.cc:474: scoped_refptr<VASurface> > > > > va_surface; > > > > On 2014/12/29 10:41:51, Pawel Osciak wrote: > > > > > On 2014/12/29 02:30:34, llandwerlin wrote: > > > > > > On 2014/12/29 00:08:01, Pawel Osciak wrote: > > > > > > > This should go after VA_SUCCESS_OR_RETURN. > > > > > > > > > > > > This is used by VA_SUCCESS_OR_RETURN. > > > > > > > > > > Ah funny, I didn't even notice that, since obviously it shouldn't. > > > > > VA_SUCCESS_OR_RETURN should instead be on va_res from line above. > > > > > > > > My... are my ears red now. Of course this is totally correct and I'm very > > > > embarrassingly wrong. I'm really sorry! > > > > > > No worries! > > > > Thanks! I think we only have the const issue remaining. > > (And of course the build issue Alex mentioned above) Hopefully this last change fixes everything.
lgtm % nit https://codereview.chromium.org/825163004/diff/100001/content/common/gpu/medi... File content/common/gpu/media/vaapi_wrapper.h (right): https://codereview.chromium.org/825163004/diff/100001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.h:76: // |num_va_attribs| attributes from |va_attribs|. The ownership of num_va_attribs not used anymore.
On 2014/12/30 13:04:06, Pawel Osciak wrote: > lgtm % nit > > https://codereview.chromium.org/825163004/diff/100001/content/common/gpu/medi... > File content/common/gpu/media/vaapi_wrapper.h (right): > > https://codereview.chromium.org/825163004/diff/100001/content/common/gpu/medi... > content/common/gpu/media/vaapi_wrapper.h:76: // |num_va_attribs| attributes from > |va_attribs|. The ownership of > num_va_attribs not used anymore. Thanks, fixed. Looks like the trybots are about to pass.
Not sure why the dbg build didn't work this time. I recompiled gfx_unittests without this patch and it failed to launch with the same error : undefined symbol: _ZN2ui13OzonePlatform11GetInstanceEv I suppose something else is broken.
On 2014/12/30 18:11:52, llandwerlin wrote: > Not sure why the dbg build didn't work this time. > I recompiled gfx_unittests without this patch and it failed to launch with the > same error : > > undefined symbol: _ZN2ui13OzonePlatform11GetInstanceEv > > I suppose something else is broken. Sigh, it could be an unrelated change that broke this. I'm trying to get ozone bots to be CQ blocking to prevent this kinda thing. I'm on the phone away from the computer so I can't look in too much detail, but it may be a gn related issue. I recall Danny fixed something similar here. https://codereview.chromium.org/818793002/
The CQ bit was checked by posciak@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/825163004/120001
The CQ bit was unchecked by posciak@chromium.org
alexst@chromium.org changed reviewers: + sadrul@chromium.org
+sadrul for
Missing LGTM from OWNERS of dependencies added to DEPS:
'+ui/display/chromeos',
'+ui/platform_window',
On 2015/01/02 14:33:05, alexst(OOTO until 2015) wrote: > +sadrul for > > Missing LGTM from OWNERS of dependencies added to DEPS: > '+ui/display/chromeos', > '+ui/platform_window', LGTM
The CQ bit was checked by spang@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/825163004/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
On 2015/01/02 16:49:57, I haz the power (commit-bot) wrote: > Try jobs failed on following builders: > chromium_presubmit on tryserver.chromium.linux > (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) TBR jln and scherkus, who already reviewed this at http://crrev.com/490233002
The CQ bit was checked by alexst@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/825163004/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/b056abc3ccfaaa7da280084ce55724a0f07d85a0 Cr-Commit-Position: refs/heads/master@{#309863} |
