Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9346012:
Video decode in hardware on ARM platform. (Closed)
|
Created:
8 years, 10 months ago by Arun Modified:
8 years, 9 months ago Reviewers:
Ami GONE FROM CHROMIUM CC:
chromium-reviews, hclam+watch_chromium.org, ddorwin+watch_chromium.org, fischman+watch_chromium.org, jam, acolwell+watch_chromium.org, annacc+watch_chromium.org, apatrick_chromium, joi+watch-content_chromium.org, darin-cc_chromium.org, vrk (LEFT CHROMIUM), scherkus (not reviewing), ihf+watch_chromium.org, micahc Base URL:
http://src.chromium.org/svn/trunk/src/ Visibility:
Public. |
DescriptionDescription: Video decode in hardware on ARM platform.
This patch modifies the GpuVideoDecodeAccelerator machinery to enable HW decode on certain platforms.
In case of some ARM platforms, the texture memory allocated by GPU(glTexImage2D)
cannot be shared with other devices such as a seperate HW decoder.
So a Pixmap buffer is allocated and an EGL Image is created from the pixmap handle.
This EGL Image is sharable between the HW Decoder and the GPU.
TEST=omx_video_decode_accelerator_unittest
Patch Set 1 #
Total comments: 12
Patch Set 2 : '' #
Total comments: 13
Patch Set 3 : '' #
Total comments: 7
Patch Set 4 : '' #
Total comments: 1
Patch Set 5 : '' #Patch Set 6 : '' #
Total comments: 3
Patch Set 7 : '' #
Total comments: 2
Patch Set 8 : '' #Patch Set 9 : #
Messages
Total messages: 47 (0 generated)
Hi This patch is related the buffer allocation and sharing only. To test the unit test on our platform(Exynos), I had to make few more changes in OpenMax integration. Please review the same.
Hi Arun, Are you or your company represented in the AUTHORS file, and have you/has it signed the CLA <http://code.google.com/legal/individual-cla-v1.0.html> / CCLA <http://code.google.com/legal/corporate-cla-v1.0.html>? Cheers, -a On Mon, Feb 6, 2012 at 8:58 PM, <arunm.chrome@gmail.com> wrote: > Reviewers: Ami Fischman, > > Message: > Hi > > This patch is related the buffer allocation and sharing only. To test the > unit > test on our platform(Exynos), I had to make few more changes in OpenMax > integration. > > Please review the same. > > Description: > Description: Video decode in hardware on ARM platform. > > This patch modifies the GpuVideoDecodeAccelerator machinery to enable HW > decode > on certain platforms. > > In case of some ARM platforms, the texture memory allocated by > GPU(glTexImage2D) > cannot be shared with other devices such as a seperate HW decoder. > So a Pixmap buffer is allocated and an EGL Image is created from the pixmap > handle. > This EGL Image is sharable between the HW Decoder and the GPU. > > TEST=omx_video_decode_**accelerator_unittest > > Please review this at https://chromiumcodereview.**appspot.com/9346012/<https://chromiumcodereview.... > > SVN Base: http://src.chromium.org/svn/**trunk/src/<http://src.chromium.org/svn/trunk/src/> > > Affected files: > M content/common/gpu/media/**gles2_texture_to_egl_image_** > translator.h > M content/common/gpu/media/**gles2_texture_to_egl_image_** > translator.cc > M content/common/gpu/media/gpu_**video_decode_accelerator.cc > M content/common/gpu/media/omx_**video_decode_accelerator.h > M content/common/gpu/media/omx_**video_decode_accelerator.cc > M content/common/gpu/media/**video_decode_accelerator_**unittest.cc > > >
Hi Ami I signed the individual CLA today. My company has started the procedure to sign the CCLA. Regards Arun On 2012/02/07 05:11:41, Ami Fischman wrote: > Hi Arun, > Are you or your company represented in the AUTHORS file, and have you/has > it signed the CLA <http://code.google.com/legal/individual-cla-v1.0.html> / > CCLA <http://code.google.com/legal/corporate-cla-v1.0.html%3E? > > Cheers, > -a > > On Mon, Feb 6, 2012 at 8:58 PM, <mailto:arunm.chrome@gmail.com> wrote: > > > Reviewers: Ami Fischman, > > > > Message: > > Hi > > > > This patch is related the buffer allocation and sharing only. To test the > > unit > > test on our platform(Exynos), I had to make few more changes in OpenMax > > integration. > > > > Please review the same. > > > > Description: > > Description: Video decode in hardware on ARM platform. > > > > This patch modifies the GpuVideoDecodeAccelerator machinery to enable HW > > decode > > on certain platforms. > > > > In case of some ARM platforms, the texture memory allocated by > > GPU(glTexImage2D) > > cannot be shared with other devices such as a seperate HW decoder. > > So a Pixmap buffer is allocated and an EGL Image is created from the pixmap > > handle. > > This EGL Image is sharable between the HW Decoder and the GPU. > > > > TEST=omx_video_decode_**accelerator_unittest > > > > Please review this at > https://chromiumcodereview.**appspot.com/9346012/%3Chttps://chromiumcoderevie...> > > > > SVN Base: > http://src.chromium.org/svn/**trunk/src/%3Chttp://src.chromium.org/svn/trunk/...> > > > > Affected files: > > M content/common/gpu/media/**gles2_texture_to_egl_image_** > > translator.h > > M content/common/gpu/media/**gles2_texture_to_egl_image_** > > translator.cc > > M content/common/gpu/media/gpu_**video_decode_accelerator.cc > > M content/common/gpu/media/omx_**video_decode_accelerator.h > > M content/common/gpu/media/omx_**video_decode_accelerator.cc > > M content/common/gpu/media/**video_decode_accelerator_**unittest.cc > > > > > >
Thanks for working on this. As I said in a comment below I very much hope you can contain the changes to an implementation .cc file instead of having the GLES2 impl dictate API choices. Can you say more in the CL description about the HW/libs combination that makes this necessary? (note that since it's unlikely we have that hardware on in the various bot farms it's unlikely that the new codepaths will get automated testing; I'm not sure how to deal with that risk to regression / bit-rot) https://chromiumcodereview.appspot.com/9346012/diff/1/content/common/gpu/medi... File content/common/gpu/media/gles2_texture_to_egl_image_translator.cc (right): https://chromiumcodereview.appspot.com/9346012/diff/1/content/common/gpu/medi... content/common/gpu/media/gles2_texture_to_egl_image_translator.cc:16: static PFNGLEGLIMAGETARGETTEXTURE2DOESPROC glEGL_ImageTargetTexture2DOES = please follow the naming conventions above. https://chromiumcodereview.appspot.com/9346012/diff/1/content/common/gpu/medi... content/common/gpu/media/gles2_texture_to_egl_image_translator.cc:18: eglGetProcAddress("glEGLImageTargetTexture2DOES")); This is an extension that's not guaranteed to exist on all GLES2 impls, right? That means you need to make it conditional. https://chromiumcodereview.appspot.com/9346012/diff/1/content/common/gpu/medi... content/common/gpu/media/gles2_texture_to_egl_image_translator.cc:22: && glEGL_ImageTargetTexture2DOES); && goes on previous line, and indent is wrong. Please see http://dev.chromium.org/developers/coding-style for chromium's coding style. There are lots of style violations in this CL; I'll let you read that doc & make a pass fixing them up instead of pointing them all out individually. https://chromiumcodereview.appspot.com/9346012/diff/1/content/common/gpu/medi... content/common/gpu/media/gles2_texture_to_egl_image_translator.cc:65: glBindTexture(GL_TEXTURE_2D, texture); Given the calls below (which seem to bind the eglImage to the pixmap) I don't understand the need for the gl calls here and the eglMakeCurrent call above (and thus for the surface param). Do you have an API reference I can look at to understand why this is necessary? https://chromiumcodereview.appspot.com/9346012/diff/1/content/common/gpu/medi... content/common/gpu/media/gles2_texture_to_egl_image_translator.cc:105: for(i=0; i<pixmap_.size(); i++) IIUC, this is completely broken. Why not instead store a map<EGLImageKHR, Pixmap> that would allow you to do the lookup/free/remove dance in a straightforward manner? https://chromiumcodereview.appspot.com/9346012/diff/1/content/common/gpu/medi... File content/common/gpu/media/gles2_texture_to_egl_image_translator.h (right): https://chromiumcodereview.appspot.com/9346012/diff/1/content/common/gpu/medi... content/common/gpu/media/gles2_texture_to_egl_image_translator.h:8: #include <utility> unneeded https://chromiumcodereview.appspot.com/9346012/diff/1/content/common/gpu/medi... content/common/gpu/media/gles2_texture_to_egl_image_translator.h:10: #include "base/scoped_ptr.h" unneeded https://chromiumcodereview.appspot.com/9346012/diff/1/content/common/gpu/medi... content/common/gpu/media/gles2_texture_to_egl_image_translator.h:21: #include <X11/extensions/Xcomposite.h> None of these includes are necessary, are they? (fwd-declare Display if needed) https://chromiumcodereview.appspot.com/9346012/diff/1/content/common/gpu/medi... content/common/gpu/media/gles2_texture_to_egl_image_translator.h:40: EGLImageKHR CreateEglImage(Display* x_display, EGLDisplay egl_display, Having a "Create" call per underlying implementation is uncool. It would be best to fold the implementation details into the .cc file, and present just a single creation API. https://chromiumcodereview.appspot.com/9346012/diff/1/content/common/gpu/medi... File content/common/gpu/media/gpu_video_decode_accelerator.cc (right): https://chromiumcodereview.appspot.com/9346012/diff/1/content/common/gpu/medi... content/common/gpu/media/gpu_video_decode_accelerator.cc:140: video_decoder->SetEglState(gfx::GLSurfaceEGL::GetNativeDisplay(), The fact that this param doesn't depend on any state special to the call-site, and that this is the only call-site, smells to me like an indication that the parameter is unnecessary; it could just as well be computed in o_v_d_a.cc https://chromiumcodereview.appspot.com/9346012/diff/1/content/common/gpu/medi... File content/common/gpu/media/omx_video_decode_accelerator.cc (right): https://chromiumcodereview.appspot.com/9346012/diff/1/content/common/gpu/medi... content/common/gpu/media/omx_video_decode_accelerator.cc:25: int height_ = 0; No. These are mutable static globals, and are a bad idea. (at the very least, they'll cause huge bugs when multiple decoders are used concurrently w/ different-sized videos). https://chromiumcodereview.appspot.com/9346012/diff/1/content/common/gpu/medi... content/common/gpu/media/omx_video_decode_accelerator.cc:210: "OMX.SEC.AVC.Decoder"); What is "SEC"? can you add a comment identifying the chipset/mfr or other relevant info? (ideally, link to their OMX/GLES2 docs)
Thanks for all the review comments. This is the initial code, so I will make the changes regarding coding style in the next pass. > glBindTexture(GL_TEXTURE_2D, texture); > Given the calls below (which seem to bind the eglImage to the pixmap) I don't > understand the need for the gl calls here and the eglMakeCurrent call above (and > thus for the surface param). Do you have an API reference I can look at to > understand why this is necessary? glEGLImageTargetTexture2DOES binds the EGL Image to the current active texture. So glBindTexture is required before glEGLImageTargetTexture2DOES is called. eglMakeCurrent maybe required when there are multiple contexts. Currently, I have tested only single decoder. Will check if it is really required when i test multiple instance / multiple tabs playing video. On 2012/02/07 17:05:40, Ami Fischman wrote: > Thanks for working on this. > > As I said in a comment below I very much hope you can contain the changes to an > implementation .cc file instead of having the GLES2 impl dictate API choices. > > Can you say more in the CL description about the HW/libs combination that makes > this necessary? > > (note that since it's unlikely we have that hardware on in the various bot farms > it's unlikely that the new codepaths will get automated testing; I'm not sure > how to deal with that risk to regression / bit-rot) > > https://chromiumcodereview.appspot.com/9346012/diff/1/content/common/gpu/medi... > File content/common/gpu/media/gles2_texture_to_egl_image_translator.cc (right): > > https://chromiumcodereview.appspot.com/9346012/diff/1/content/common/gpu/medi... > content/common/gpu/media/gles2_texture_to_egl_image_translator.cc:16: static > PFNGLEGLIMAGETARGETTEXTURE2DOESPROC glEGL_ImageTargetTexture2DOES = > please follow the naming conventions above. > > https://chromiumcodereview.appspot.com/9346012/diff/1/content/common/gpu/medi... > content/common/gpu/media/gles2_texture_to_egl_image_translator.cc:18: > eglGetProcAddress("glEGLImageTargetTexture2DOES")); > This is an extension that's not guaranteed to exist on all GLES2 impls, right? > That means you need to make it conditional. > > https://chromiumcodereview.appspot.com/9346012/diff/1/content/common/gpu/medi... > content/common/gpu/media/gles2_texture_to_egl_image_translator.cc:22: && > glEGL_ImageTargetTexture2DOES); > && goes on previous line, and indent is wrong. > Please see http://dev.chromium.org/developers/coding-style for chromium's coding > style. > There are lots of style violations in this CL; I'll let you read that doc & make > a pass fixing them up instead of pointing them all out individually. > > https://chromiumcodereview.appspot.com/9346012/diff/1/content/common/gpu/medi... > content/common/gpu/media/gles2_texture_to_egl_image_translator.cc:65: > glBindTexture(GL_TEXTURE_2D, texture); > Given the calls below (which seem to bind the eglImage to the pixmap) I don't > understand the need for the gl calls here and the eglMakeCurrent call above (and > thus for the surface param). Do you have an API reference I can look at to > understand why this is necessary? > > https://chromiumcodereview.appspot.com/9346012/diff/1/content/common/gpu/medi... > content/common/gpu/media/gles2_texture_to_egl_image_translator.cc:105: for(i=0; > i<pixmap_.size(); i++) > IIUC, this is completely broken. > Why not instead store a map<EGLImageKHR, Pixmap> that would allow you to do the > lookup/free/remove dance in a straightforward manner? > > https://chromiumcodereview.appspot.com/9346012/diff/1/content/common/gpu/medi... > File content/common/gpu/media/gles2_texture_to_egl_image_translator.h (right): > > https://chromiumcodereview.appspot.com/9346012/diff/1/content/common/gpu/medi... > content/common/gpu/media/gles2_texture_to_egl_image_translator.h:8: #include > <utility> > unneeded > > https://chromiumcodereview.appspot.com/9346012/diff/1/content/common/gpu/medi... > content/common/gpu/media/gles2_texture_to_egl_image_translator.h:10: #include > "base/scoped_ptr.h" > unneeded > > https://chromiumcodereview.appspot.com/9346012/diff/1/content/common/gpu/medi... > content/common/gpu/media/gles2_texture_to_egl_image_translator.h:21: #include > <X11/extensions/Xcomposite.h> > None of these includes are necessary, are they? > (fwd-declare Display if needed) > > https://chromiumcodereview.appspot.com/9346012/diff/1/content/common/gpu/medi... > content/common/gpu/media/gles2_texture_to_egl_image_translator.h:40: EGLImageKHR > CreateEglImage(Display* x_display, EGLDisplay egl_display, > Having a "Create" call per underlying implementation is uncool. > It would be best to fold the implementation details into the .cc file, and > present just a single creation API. > > https://chromiumcodereview.appspot.com/9346012/diff/1/content/common/gpu/medi... > File content/common/gpu/media/gpu_video_decode_accelerator.cc (right): > > https://chromiumcodereview.appspot.com/9346012/diff/1/content/common/gpu/medi... > content/common/gpu/media/gpu_video_decode_accelerator.cc:140: > video_decoder->SetEglState(gfx::GLSurfaceEGL::GetNativeDisplay(), > The fact that this param doesn't depend on any state special to the call-site, > and that this is the only call-site, smells to me like an indication that the > parameter is unnecessary; it could just as well be computed in o_v_d_a.cc > > https://chromiumcodereview.appspot.com/9346012/diff/1/content/common/gpu/medi... > File content/common/gpu/media/omx_video_decode_accelerator.cc (right): > > https://chromiumcodereview.appspot.com/9346012/diff/1/content/common/gpu/medi... > content/common/gpu/media/omx_video_decode_accelerator.cc:25: int height_ = 0; > No. These are mutable static globals, and are a bad idea. > (at the very least, they'll cause huge bugs when multiple decoders are used > concurrently w/ different-sized videos). > > https://chromiumcodereview.appspot.com/9346012/diff/1/content/common/gpu/medi... > content/common/gpu/media/omx_video_decode_accelerator.cc:210: > "OMX.SEC.AVC.Decoder"); > What is "SEC"? can you add a comment identifying the chipset/mfr or other > relevant info? > (ideally, link to their OMX/GLES2 docs)
https://chromiumcodereview.appspot.com/9346012/diff/1/content/common/gpu/medi... > content/common/gpu/media/gpu_video_decode_accelerator.cc:140: > video_decoder->SetEglState(gfx::GLSurfaceEGL::GetNativeDisplay(), > The fact that this param doesn't depend on any state special to the call-site, > and that this is the only call-site, smells to me like an indication that the > parameter is unnecessary; it could just as well be computed in o_v_d_a.cc > Calling gfx::GLSurfaceEGL::GetNativeDisplay() from o_v_d_a.cc will fail for unit test. So I think its better to add the native display parameter to SetEglState.
> > glEGLImageTargetTexture2DOES binds the EGL Image to the current active > texture. > So glBindTexture is required before glEGLImageTargetTexture2DOES is > called. > I'm confused; IIUC egl_create_image_khr(...EGL_NATIVE_PIXMAP_KHR...) creates an EGLImage wrapping the passed-in Pixmap, so drawing to the EGLImage (which is what the decoder is given) will actually draw to the pixmap's memory. In that case what does it mean to "bind the EGL Image to a texture"? Will the decoder write to both the texture's memory and the pixmap? Or do you mean it binds the other way, replacing the texture's memory w/ the pixmap's memory under the covers so future accesses to the texture see the memory the decoder wrote? BTW what fails on this platform using the ToT code? > Calling gfx::GLSurfaceEGL::**GetNativeDisplay() from o_v_d_a.cc will fail > for unit test. So I think its better to add the native display parameter to > SetEglState. Underneath all the layers and indirections, doesn't it all basically come down to XOpenDisplay(NULL) both in the test & non-test codepaths?
> I'm confused; IIUC > egl_create_image_khr(...EGL_NATIVE_PIXMAP_KHR...) creates an EGLImage > wrapping the passed-in Pixmap, so drawing to the EGLImage (which is what > the decoder is given) will actually draw to the pixmap's memory. In that > case what does it mean to "bind the EGL Image to a texture"? Will the > decoder write to both the texture's memory and the pixmap? > Or do you mean it binds the other way, replacing the texture's memory w/ > the pixmap's memory under the covers so future accesses to the texture see > the memory the decoder wrote? > Yes, glEGLImageTargetTexture2DOES replaces the texture's memory w/ the pixmap's memory under the covers so future accesses to the texture see the memory the decoder wrote. So, we dont have to copy the decoder output to the texture memory. Once this is done, the textures can be rendered as before.
> > Yes, glEGLImageTargetTexture2DOES replaces the texture's memory w/ the > pixmap's > memory under the covers so future accesses to the texture see the memory > the > decoder wrote. So, we dont have to copy the decoder output to the texture > memory. Once this is done, the textures can be rendered as before. > Interesting. Can most/all of this CL be replaced by making the original texture creation use share-able memory?
> > Interesting. Can most/all of this CL be replaced by making the original > texture creation use share-able memory? For PPAPI and media player, glTexImage2D is used to create the memory. This memory is allocated by the GPU and not accessible to any other device. On platforms where GPU and decoder are on the same device, the original code will work. But on many other platforms, GPU and HW decoder are from different vendors. We had forwarded few proposals to the OS team. They had rejected the idea to add a new API to allocate a video texture which use share-able memory. Antoine had said that the Pixmap method was the most promising. Katie Roberts will have the complete doc explaining all the proposals. So I had made the above changes such that only files in the gpu/media folder are modified.
Caught up with mtg notes explaining the decision process. Ignore my comment about glEGLImageTargetTexture2DOES.
I have changed the code according the the comments given above.
Nice! https://chromiumcodereview.appspot.com/9346012/diff/13001/content/common/gpu/... File content/common/gpu/media/gles2_texture_to_egl_image_translator.cc (right): https://chromiumcodereview.appspot.com/9346012/diff/13001/content/common/gpu/... content/common/gpu/media/gles2_texture_to_egl_image_translator.cc:45: if (x_display_) { This seems like a strange way to indicate which codepath to go down. Why not pass an explicit bool use_backing_pixmaps parameter to the ctor instead of x_display? Also, there's no reason OVDA should call gfx::GLSurfaceEGL::GetNativeDisplay() just to pass it to here, when this class could just as well call the same function. https://chromiumcodereview.appspot.com/9346012/diff/13001/content/common/gpu/... content/common/gpu/media/gles2_texture_to_egl_image_translator.cc:66: eglimage_pixmap_.insert(std::make_pair(hEglImage, pixmap)); If you expect no collisions, the standard idiom is to say so with: bool inserted = the_map.insert(std::make_pair(k, v)).second; DCHECK(inserted); https://chromiumcodereview.appspot.com/9346012/diff/13001/content/common/gpu/... content/common/gpu/media/gles2_texture_to_egl_image_translator.cc:76: CHECK(hEglImage) << "Failed to eglCreateImageKHR for " << texture This CHECK and the one at l.62 can be coallesced into a single CHECK outside the if/then/else below. https://chromiumcodereview.appspot.com/9346012/diff/13001/content/common/gpu/... content/common/gpu/media/gles2_texture_to_egl_image_translator.cc:105: Pixmap pixmap = it->second; Also eglimage_pixmap_.erase(it); ? https://chromiumcodereview.appspot.com/9346012/diff/13001/content/common/gpu/... content/common/gpu/media/gles2_texture_to_egl_image_translator.cc:106: if (pixmap) When will this test be false? https://chromiumcodereview.appspot.com/9346012/diff/13001/content/common/gpu/... File content/common/gpu/media/gles2_texture_to_egl_image_translator.h (right): https://chromiumcodereview.appspot.com/9346012/diff/13001/content/common/gpu/... content/common/gpu/media/gles2_texture_to_egl_image_translator.h:15: #include "third_party/mesa/MesaLib/include/GL/glext.h" What's the deal with the s/angle/mesa/ here and above? https://chromiumcodereview.appspot.com/9346012/diff/13001/content/common/gpu/... File content/common/gpu/media/omx_video_decode_accelerator.cc (right): https://chromiumcodereview.appspot.com/9346012/diff/13001/content/common/gpu/... content/common/gpu/media/omx_video_decode_accelerator.cc:16: #if (defined(OS_CHROMEOS) && defined(ARCH_CPU_ARMEL)) No other configuration builds this .cc file so IMO this guard is unnecessary. https://chromiumcodereview.appspot.com/9346012/diff/13001/content/common/gpu/... content/common/gpu/media/omx_video_decode_accelerator.cc:361: texture2eglImage_translator_ = This is leaking a translator per call to AssignPictureBuffers. I think you want to make the class member a scoped_ptr<Gles2TextureToEglImageTranslator> instead of a raw pointer. Also, instead of creating a translator per call to AssignPictureBuffers, you can create it once in CreateComponent(), which will allow you to remove the component_name_is_sec_h264ext_ member (in favor of a variable local to the CreateComponent method). https://chromiumcodereview.appspot.com/9346012/diff/13001/content/common/gpu/... File content/common/gpu/media/omx_video_decode_accelerator.h (right): https://chromiumcodereview.appspot.com/9346012/diff/13001/content/common/gpu/... content/common/gpu/media/omx_video_decode_accelerator.h:22: #include "content/common/gpu/media/gles2_texture_to_egl_image_translator.h" Since only a pointer to the translator is held by this class, you can fwd-declare it and avoid the extra header inclusions. https://chromiumcodereview.appspot.com/9346012/diff/13001/content/common/gpu/... content/common/gpu/media/omx_video_decode_accelerator.h:150: int width_ ; prefix both this and the next member to indicate their relevance: int last_requested_picture_buffer_width_; int last_requested_picture_buffer_height_; Although I'd use a gfx::Size instead: gfx::Size last_requested_picture_buffer_dimensions_; and also pass a const gfx::Size& to the translator's method instead of independent width/height params. (using a single variable ensures the order is not confused, and ensures that both width & height are always set and neither is forgotten) https://chromiumcodereview.appspot.com/9346012/diff/13001/content/common/gpu/... content/common/gpu/media/omx_video_decode_accelerator.h:184: Gles2TextureToEglImageTranslator* texture2eglImage_translator_; we don't use '2' as a synonym for 'to', and what we don't use camelCase for class members. texture_to_egl_image_translator_ https://chromiumcodereview.appspot.com/9346012/diff/13001/content/common/gpu/... content/common/gpu/media/omx_video_decode_accelerator.h:186: // These two members are only used during Initialization. s/two // https://chromiumcodereview.appspot.com/9346012/diff/13001/content/common/gpu/... File content/common/gpu/media/video_decode_accelerator_unittest.cc (right): https://chromiumcodereview.appspot.com/9346012/diff/13001/content/common/gpu/... content/common/gpu/media/video_decode_accelerator_unittest.cc:48: #include "third_party/angle/include/EGL/egl.h" I think this move should be reverted if you take my suggestion elsewhere. (and, I think, as a result the file will drop out of this CL naturally)
Patch 3 submitted. component_name_is_sec_h264ext_ is required for some changes in OMX integration. I will submit the patch regarding OMX changes once this review is complete.
Almost there. https://chromiumcodereview.appspot.com/9346012/diff/19002/content/common/gpu/... File content/common/gpu/media/gles2_texture_to_egl_image_translator.cc (right): https://chromiumcodereview.appspot.com/9346012/diff/19002/content/common/gpu/... content/common/gpu/media/gles2_texture_to_egl_image_translator.cc:28: if (!AreEGLExtensionsInitialized()) { What happens if glegl_image_targettexture_2does is NULL? I *think* glEGLImageTargetTexture2DOES is expected to always be present on the platforms this code runs on (tegra & exynos), meaning glegl_image_targettexture_2does should be part of AreEGLExtensionsInitialized(). If that's wrong (it's an optional extension), then I guess the if statement here wants to be something like: if (!AreEGLExtensionsInitialized() || (use_backing_pixmaps_ && !glegl_image_targettexture_2does)) { ... https://chromiumcodereview.appspot.com/9346012/diff/19002/content/common/gpu/... File content/common/gpu/media/gles2_texture_to_egl_image_translator.h (right): https://chromiumcodereview.appspot.com/9346012/diff/19002/content/common/gpu/... content/common/gpu/media/gles2_texture_to_egl_image_translator.h:34: gfx::Size* dimensions); chromium style requires input-only parameters to be passed via const-ref, not pointer: const gfx::Size& dimensions https://chromiumcodereview.appspot.com/9346012/diff/19002/content/common/gpu/... File content/common/gpu/media/omx_video_decode_accelerator.cc (right): https://chromiumcodereview.appspot.com/9346012/diff/19002/content/common/gpu/... content/common/gpu/media/omx_video_decode_accelerator.cc:112: last_requested_picture_buffer_dimensions_(gfx::Size(-1, -1)), gfx::Size()'s default ctor initializes with 0's, and there's no need for a value to indicate "uninitialized" in the code in this file, so I would just drop this line and take the implicit default construction. https://chromiumcodereview.appspot.com/9346012/diff/19002/content/common/gpu/... content/common/gpu/media/omx_video_decode_accelerator.cc:204: component_name_is_sec_h264ext_ = !strcmp( > component_name_is_sec_h264ext_ is required for some changes in OMX > integration. I will submit the patch regarding OMX changes once > this review is complete. In the interest of keeping CL's minimal, can you remove this member (replace with a local variable here) in this CL and add it in the CL that requires it? https://chromiumcodereview.appspot.com/9346012/diff/19002/content/common/gpu/... content/common/gpu/media/omx_video_decode_accelerator.cc:358: texture_to_egl_image_translator_->TranslateToEglImage( indentation is wrong here; it should be 4 spaces from the initial 'E' of the previous line (and subsequent lines will have to be reindented, as well). https://chromiumcodereview.appspot.com/9346012/diff/19002/content/common/gpu/... content/common/gpu/media/omx_video_decode_accelerator.cc:718: it->second.egl_image); indent is off https://chromiumcodereview.appspot.com/9346012/diff/19002/content/common/gpu/... File content/common/gpu/media/omx_video_decode_accelerator.h (right): https://chromiumcodereview.appspot.com/9346012/diff/19002/content/common/gpu/... content/common/gpu/media/omx_video_decode_accelerator.h:188: // These two members are only used during Initialization. I think you missed my comment about dropping the "two" from this comment (now that it refers to three members).
Patch 4 submitted.
Almost ready to land; other than the one nit below, there's just the question of the CCLA/CLA and AUTHORS file. I'm waiting to hear back on this for the final go-ahead. https://chromiumcodereview.appspot.com/9346012/diff/20005/content/common/gpu/... File content/common/gpu/media/gles2_texture_to_egl_image_translator.h (right): https://chromiumcodereview.appspot.com/9346012/diff/20005/content/common/gpu/... content/common/gpu/media/gles2_texture_to_egl_image_translator.h:22: // TODO(fischman): this class is now stateless (it wasn't always). If it seems Please remove this TODO paragraph (l.22-24) now that it no longer applies.
Thanks for reviewing the code.
On 2012/02/22 02:29:24, Ami Fischman wrote: > Almost ready to land; other than the one nit below, there's just the question of > the CCLA/CLA and AUTHORS file. I'm waiting to hear back on this for the final > go-ahead. Micah had told us that the CCLA signed sometime back is still valid. Can you please check the same.
On 2012/02/22 11:34:28, Arun wrote: > Micah had told us that the CCLA signed sometime back is still valid. Can you > please check the same. Yep; working w/ Micah to verify everything's in order and will land the patch when confirmed. Thanks!
I have modified the o_v_d_a.cc for some OMX related changes. The main change is the fact that currently the color conversion HW/device driver supports only 3 output buffers. Also as I had commented earlier gfx::GLSurfaceEGL::GetNativeDisplay() returns NULL in case of unittest but it returns a valid handle incase of video playback through browser. In case of unittest, XOpenDisplay is called in v_d_a_u.cc and not in gl_surface_egl.cc Please send me your comments, as I can submit the next patch accordingly.
On 2012/03/05 09:59:22, Arun wrote: > I have modified the o_v_d_a.cc for some OMX related changes. Did you forget to upload the new patch to rietveld? I don't see these changes in patchset#5, the most recent patchset. > The main change is > the fact that currently the color conversion HW/device driver supports only 3 > output buffers. Does that work with <video>? I'm worried about kMaxVideoFrames being 4 in the media/ pipeline (http://code.google.com/codesearch#OAMlx_jo-ck/src/media/base/limits.h&exact_p...). > Also as I had commented earlier gfx::GLSurfaceEGL::GetNativeDisplay() returns > NULL in case of unittest but it returns a valid handle incase of video playback > through browser. > In case of unittest, XOpenDisplay is called in v_d_a_u.cc and not in > gl_surface_egl.cc > Please send me your comments, as I can submit the next patch accordingly. I think there's no reason to use gfx::GLSurfaceEGL::GetNativeDisplay() in what is very OMX/X11-specific code. Instead just use XOpenDisplay(NULL) directly (which is what gfx::GLSurfaceEGL::GetNativeDisplay() ends up doing, anyway).
I am yet to submit the patch for review. Just wanted to discuss the same before sending the patch. > Does that work with <video>? I'm worried about kMaxVideoFrames being 4 in the > media/ pipeline > (http://code.google.com/codesearch#OAMlx_jo-ck/src/media/base/limits.h&exact_p...). Yes, I had to modify the kMaxVideoFrames(limits.h) and kNumPictureBuffers(o_v_d_a.cc) values to 3. > > I think there's no reason to use gfx::GLSurfaceEGL::GetNativeDisplay() in what > is very OMX/X11-specific code. Instead just use XOpenDisplay(NULL) directly > (which is what gfx::GLSurfaceEGL::GetNativeDisplay() ends up doing, anyway). I am still confused about this. Do I have to replace the gfx::GLSurfaceEGL::GetNativeDisplay() calls in gles2_texture_to_egl_image_translator.cc to XOpenDisplay(NULL)?
> > Does that work with <video>? I'm worried about kMaxVideoFrames being 4 in >> the >> media/ pipeline (http://code.google.com/**codesearch#OAMlx_jo-ck/src/** >> media/base/limits.h&exact_**package=chromium&q=file:media/** >> base/limits.h&type=cs&l=23<http://code.google.com/codesearch#OAMlx_jo-ck/src/media/base/limits.h&exact_package=chromium&q=file:media/base/limits.h&type=cs&l=23> >> ). > > Yes, I had to modify the kMaxVideoFrames(limits.h) and > kNumPictureBuffers(o_v_d_a.cc) values to 3. Hmm; we'll have to test that a fair bit; the "4" has been there for a long time. > I think there's no reason to use gfx::GLSurfaceEGL::**GetNativeDisplay() >> in what >> is very OMX/X11-specific code. Instead just use XOpenDisplay(NULL) >> directly >> (which is what gfx::GLSurfaceEGL::**GetNativeDisplay() ends up doing, >> anyway). >> > I am still confused about this. > Do I have to replace the gfx::GLSurfaceEGL::**GetNativeDisplay() calls in > gles2_texture_to_egl_image_**translator.cc to XOpenDisplay(NULL)? > Yes, I think that's the most reasonable course of action. -a
> > Hmm; we'll have to test that a fair bit; the "4" has been there for a long > time. > we have solved the issue with 4 buffers in the color conversion. So kMaxVideoFrames(limits.h) and kNumPictureBuffers(o_v_d_a.cc) need not be modified.
> > we have solved the issue with 4 buffers in the color conversion. > So kMaxVideoFrames(limits.h) and kNumPictureBuffers(o_v_d_a.cc) need not be > modified. > Great. You're still planning to make the XOpenDisplay(NULL) change, though, right? -a
BTW the CCLA confirmation finally came through so this CL can land once you're done making the XOpenDisplay changes, and append a line to src/AUTHORS listing Samsung. On 2012/03/07 18:18:03, Ami Fischman wrote: > > > > we have solved the issue with 4 buffers in the color conversion. > > So kMaxVideoFrames(limits.h) and kNumPictureBuffers(o_v_d_a.cc) need not be > > modified. > > > > Great. > You're still planning to make the XOpenDisplay(NULL) change, though, right? > > -a
I have done few changes for OMX integration also. Will submit the complete patch after I do some basic testing. On 2012/03/08 17:37:20, Ami Fischman wrote: > BTW the CCLA confirmation finally came through so this CL can land once you're > done making the XOpenDisplay changes, and append a line to src/AUTHORS listing > Samsung. > > > On 2012/03/07 18:18:03, Ami Fischman wrote: > > > > > > we have solved the issue with 4 buffers in the color conversion. > > > So kMaxVideoFrames(limits.h) and kNumPictureBuffers(o_v_d_a.cc) need not be > > > modified. > > > > > > > Great. > > You're still planning to make the XOpenDisplay(NULL) change, though, right? > > > > -a
Patch 6 submitted. I have added few changes required for SEC OMX integration also. Creating Pixmap fails when i replaced the gfx::GLSurfaceEGL::GetNativeDisplay() calls in gles2_texture_to_egl_image_translator.cc to XOpenDisplay(NULL). So I have used base::MessagePumpForUI::GetDefaultXDisplay().
https://chromiumcodereview.appspot.com/9346012/diff/38001/content/common/gpu/... File content/common/gpu/media/omx_video_decode_accelerator.cc (right): https://chromiumcodereview.appspot.com/9346012/diff/38001/content/common/gpu/... content/common/gpu/media/omx_video_decode_accelerator.cc:25: void* omx_handle = dlopen("libOMXCore.so", RTLD_NOW); Is this capitalization open to discussion? The tegra2 chroot has libOmxCore.so as a symlink to libnvomx.so, but no libOMXCore.so. It'd be nice to avoid a cascading chain of fallbacks. https://chromiumcodereview.appspot.com/9346012/diff/38001/content/common/gpu/... content/common/gpu/media/omx_video_decode_accelerator.cc:173: static OMX_CALLBACKTYPE omx_accelerator_callbacks = { Why? https://chromiumcodereview.appspot.com/9346012/diff/38001/content/common/gpu/... content/common/gpu/media/omx_video_decode_accelerator.cc:667: NULL, output_buffer_size_); Since this buffer will never be used for anything why not use a buffer size of 0? Doing so would allow you to drop the output_buffer_size_ member, as well as collapse the NV/SEC branches (by always using AllocateBuffer instead of UseBuffer), which would let you also replace the component_name_is_sec_h264ext_ member with a local variable in the only place it's used.
Patch 7 submitted. Please review the same.
https://chromiumcodereview.appspot.com/9346012/diff/43001/AUTHORS File AUTHORS (right): https://chromiumcodereview.appspot.com/9346012/diff/43001/AUTHORS#newcode163 AUTHORS:163: Arun Mankuzhi <arun.m@samsung.com> This line should list "Samsung" instead of you personally since Samsung signed the CCLA. https://chromiumcodereview.appspot.com/9346012/diff/43001/content/common/gpu/... File content/common/gpu/media/video_decode_accelerator_unittest.cc (right): https://chromiumcodereview.appspot.com/9346012/diff/43001/content/common/gpu/... content/common/gpu/media/video_decode_accelerator_unittest.cc:77: FILE_PATH_LITERAL("test-25fps.h264:1280:720:250:258:33:175:1"); You can't change this without providing a new test-25fps.h264 file!
https://chromiumcodereview.appspot.com/9346012/diff/43001/content/common/gpu/... > content/common/gpu/media/video_decode_accelerator_unittest.cc:77: > FILE_PATH_LITERAL("test-25fps.h264:1280:720:250:258:33:175:1"); > You can't change this without providing a new test-25fps.h264 file! Iam using the same test file. Only the rendering window size is 1280x720.
> > https://chromiumcodereview.**appspot.com/9346012/diff/** > 43001/content/common/gpu/**media/video_decode_**accelerator_unittest.cc#** > newcode77<https://chromiumcodereview.appspot.com/9346012/diff/43001/content/common/gpu/media/video_decode_accelerator_unittest.cc#newcode77> > >> content/common/gpu/media/**video_decode_accelerator_**unittest.cc:77: >> FILE_PATH_LITERAL("test-25fps.**h264:1280:720:250:258:33:175:**1"); >> You can't change this without providing a new test-25fps.h264 file! > > Iam using the same test file. Only the rendering window size is 1280x720. > That doesn't make sense to me. The test runs up to 5 or 6 windows concurrently; what does that look like if each window is taking the full screen? Also these dimensions are used for creating the textures (I realize now there should be assertions that the dimensions requested in ProvidePictureBuffers match these dimensions); it is incorrect to ask for such large ones. Put another way: what happens if you revert this change? -a
AUTHORS file and v_d_a_u.cc modified.
LGTM except that: 1) The CCLA signatory is "Samsung" (without additional adjectives) and the CCLA doesn't cover all @samsung addresses (only specific contributors). Please replace the new line with just "Samsung". 2) Your patch doesn't apply cleanly (so CQ won't land it) because you need to rebase. You can rebase & upload a new patch that fixes these things and check the Commit check-box (my LGTM here is enough to let you do this even though you're not a chromium committer), or I can upload a new issue based on your patch and land that for you. Let me know if you want me to do the latter. Thanks for doing this! (tested patch locally on a tegra2 & it works fine)
Thanks. I will build the latest version and test on Exynos.
Checked the commit.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/arunm.chrome@gmail.com/9346012/45005
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/arunm.chrome@gmail.com/9346012/45005
Presubmit check for 9346012-45005 failed and returned exit status 1. Running presubmit commit checks ... ** Presubmit Messages ** If this change has an associated bug, add BUG=[bug number]. ** Presubmit Warnings ** arunm.chrome@gmail.com is not in AUTHORS file. If you are a new contributor, please visit http://www.chromium.org/developers/contributing-code and read the "Legal" section If you are a chromite, verify the contributor signed the CLA. Presubmit checks took 4.1s to calculate.
Looks like there are some issue in the commit.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/arunm.chrome@gmail.com/9346012/45005
Presubmit check for 9346012-45005 failed and returned exit status 1. Running presubmit commit checks ... ** Presubmit Messages ** If this change has an associated bug, add BUG=[bug number]. ** Presubmit Warnings ** arunm.chrome@gmail.com is not in AUTHORS file. If you are a new contributor, please visit http://www.chromium.org/developers/contributing-code and read the "Legal" section If you are a chromite, verify the contributor signed the CLA. Presubmit checks took 1.4s to calculate.
Sorry for the run-around. I checked with People Who Know(tm) and it turns out I was wrong to tell you to use Samsung in the first place, since you're using your personal account for this issue. You created this issue using your personal email address, and you've signed the individual CLA, so if you want to land it from here, then you can replace your AUTHORS edit with a line that simply reads: Arun Mankuzhi <arunm.chrome@gmail.com> and check the commit box. If you want to (or need to b/c that's what samsung wants) land this CL using your @samsung.com address, you'll have to open a new rietveld issue using that account, and then add your @samsung.com account to AUTHORS instead of your personal address. I'll have to LGTM that CL separately to let the CQ land it. In case you're curious, the backstory is that AUTHORS needs to: - list email addresses or *@ patterns (that's why a bare "Samsung" isn't useful) - list only contributors, not all people approved by their employers CCLA even if they haven't contributed (which is why we don't simply add all approved contributors when a CCLA gets signed, or when an individual CLA is signed) - list the email address used to open the rietveld issue, which is why you can't land this CL (opened with your personal account) by adding your @samsung.com address to AUTHORS
Created a new patch with @samsung.com address. https://chromiumcodereview.appspot.com/9724030/
I marked this issue as closed and L GTM'd & CQ'd the new version at https://chromiumcodereview.appspot.com/9724030/. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
