Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8510039:
Initial implementation of the DXVA 2.0 H.264 hardware decoder for pepper for Windows. The decodin... (Closed)
|
Created:
9 years, 1 month ago by ananta Modified:
9 years ago CC:
chromium-reviews, hclam+watch_chromium.org, ddorwin+watch_chromium.org, fischman+watch_chromium.org, jam, acolwell+watch_chromium.org, annacc+watch_chromium.org, dpranke-watch+content_chromium.org, joi+watch-content_chromium.org, darin-cc_chromium.org, ajwong+watch_chromium.org, vrk (LEFT CHROMIUM), scherkus (not reviewing), ihf+watch_chromium.org, dgkoch Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionInitial implementation of the DXVA 2.0 H.264 hardware decoder for pepper for Windows. The decoding is done
using the Microsoft Media Foundation API. To render the output bitmap on the GPU texture we create a temporary
Direct3D surface in the RGB format and copy the decoded contents to this surface. This will change once we
have an ANGLE extension which allows us to pass the decoded surface as is for rendering.
We do the following prior to initializing the GPU sandbox:-
1. Load necessary decoding dlls.
2. Create static instances of the IDirect3DDeviceManager9 and the IDirect3DDevice9Ex interfaces. These are shared
among all decoder instances.
This work is done in the PreSandboxInitialization function in the DXVAVideoDecodeAccelerator class.
We cannot use CoCreateInstance to instantiate the h.264 decoder as that fails in the sandbox. Instead we do the
donkey work of loading the dll and using DllGetClassObject to instantiate the decoder.
BUG=none
TEST=Refactored the omx_video_decode_accelerator_unittest.cc test to ensure it works on Windows and Chrome OS.
This file has been renamed as video_decode_accelerator_unittest.cc as it now works on both windows and cros.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=115482
Patch Set 1 #Patch Set 2 : '' #Patch Set 3 : '' #
Total comments: 11
Patch Set 4 : '' #Patch Set 5 : '' #Patch Set 6 : '' #
Total comments: 23
Patch Set 7 : '' #Patch Set 8 : '' #
Total comments: 16
Patch Set 9 : '' #Patch Set 10 : '' #Patch Set 11 : '' #
Total comments: 189
Patch Set 12 : '' #Patch Set 13 : '' #Patch Set 14 : '' #Patch Set 15 : '' #Patch Set 16 : '' #Patch Set 17 : '' #Patch Set 18 : '' #Patch Set 19 : '' #Patch Set 20 : '' #Patch Set 21 : '' #Patch Set 22 : '' #Patch Set 23 : '' #Patch Set 24 : '' #Patch Set 25 : '' #Patch Set 26 : '' #Patch Set 27 : '' #Patch Set 28 : '' #Patch Set 29 : '' #
Total comments: 151
Patch Set 30 : '' #Patch Set 31 : '' #Patch Set 32 : '' #Patch Set 33 : '' #Patch Set 34 : '' #Patch Set 35 : '' #Patch Set 36 : '' #Patch Set 37 : '' #Patch Set 38 : '' #Patch Set 39 : '' #Patch Set 40 : '' #Patch Set 41 : '' #Patch Set 42 : '' #Patch Set 43 : '' #
Total comments: 51
Patch Set 44 : '' #
Total comments: 11
Patch Set 45 : '' #Patch Set 46 : '' #
Total comments: 14
Patch Set 47 : '' #
Total comments: 8
Patch Set 48 : '' #Patch Set 49 : '' #
Total comments: 6
Patch Set 50 : '' #
Total comments: 1
Patch Set 51 : '' #Patch Set 52 : '' #
Total comments: 7
Patch Set 53 : '' #Patch Set 54 : '' #Patch Set 55 : '' #Messages
Total messages: 40 (0 generated)
I know this isn't ready for review yet but a few ideas... http://codereview.chromium.org/8510039/diff/3003/content/common/gpu/media/dxv... File content/common/gpu/media/dxva_video_decode_accelerator.cc (right): http://codereview.chromium.org/8510039/diff/3003/content/common/gpu/media/dxv... content/common/gpu/media/dxva_video_decode_accelerator.cc:21: base::LazyInstance<base::win::ScopedCOMInitializer> This happens in gpu_main.cc so it might not be necessary here. Also, I think this might need to happen before the sandbox is enabled, so if it didn't happen in gpu_main.cc, this might be too late. http://codereview.chromium.org/8510039/diff/3003/content/common/gpu/media/dxv... content/common/gpu/media/dxva_video_decode_accelerator.cc:201: ::OpenProcess(PROCESS_DUP_HANDLE, I think the sandbox might prevent OpenProcess from working. GpuChannel has a handle for the corresponding renderer process though. http://codereview.chromium.org/8510039/diff/3003/content/common/gpu/media/dxv... content/common/gpu/media/dxva_video_decode_accelerator.cc:360: d3d9_.Attach(Direct3DCreate9(D3D_SDK_VERSION)); I think it will be easier to synchronize this code with ANGLE if it used ANGLE's D3D device rather than creating its own. I was thinking about exposing the IDirect3DDeviceManager interface as an ANGLE EGL extension. You could get the D3D device by calling the equivalent of ID3DDM::LockDevice, though it would be exposed as an EGL entry point called something like eglLockDevice. http://codereview.chromium.org/8510039/diff/3003/content/common/gpu/media/dxv... content/common/gpu/media/dxva_video_decode_accelerator.cc:408: HRESULT hr = CoCreateInstance(__uuidof(CMSH264DecoderMFT), Does this work when the sandbox is enabled?
Here's an EGL extension that could allow access to the ANGLE's D3D and a renderable texture. It's based on IDirect3DDeviceManager9. So far I've got it compiling but I haven't tested it. I was thinking I could test it using your patch once you have the LockRect / glTexImage2D path working. http://codereview.appspot.com/5374087/
This is a (not yet working) prototype of a GL extension to copy an IDirect3DSurface9 into an ANGLE GL texture by texture ID, assuming its using ANGLE's D3D device. http://codereview.appspot.com/5369113/ I think with these two extensions in ANGLE we can make your patch work. Let me know what you think and I'll try and get these in ANGLE.
+daniel to look at the ANGLE side of things. http://codereview.chromium.org/8510039/diff/3003/content/common/gpu/media/dxv... File content/common/gpu/media/dxva_video_decode_accelerator.cc (right): http://codereview.chromium.org/8510039/diff/3003/content/common/gpu/media/dxv... content/common/gpu/media/dxva_video_decode_accelerator.cc:378: HRESULT hr = d3d9_->CreateDevice(D3DADAPTER_DEFAULT, Daniel, the DXVA video decoder needs a D3D device. I could adapt some of the existing surface sharing via D3D share handles but that would result in unnecessary synchronization given this code is running in the same process as ANGLE. Ideally, we would have access to ANGLE's D3D device and use the Direct3DDeviceManager mechanisms to ensure its D3D state does not get messed up and to get notification when the device has been reset. http://codereview.chromium.org/8510039/diff/3003/content/common/gpu/media/dxv... content/common/gpu/media/dxva_video_decode_accelerator.cc:422: reinterpret_cast<ULONG_PTR>(device_manager_.get())); DXVA also needs a pointer to the Direct3DDeviceManager here... http://codereview.chromium.org/8510039/diff/3003/content/common/gpu/media/dxv... content/common/gpu/media/dxva_video_decode_accelerator.cc:708: EGLImageKHR egl_image = texture2eglImage_translator.TranslateToEglImage( ... and here we have a D3D surface (not a texture) that we need to copy into a GL (ANGLE) texture by texture ID. This code doesn't work yet BTW; I think it was copied from a platform that actually supports EGLImageKHR.
http://codereview.chromium.org/8510039/diff/3003/content/common/gpu/media/dxv... File content/common/gpu/media/dxva_video_decode_accelerator.cc (right): http://codereview.chromium.org/8510039/diff/3003/content/common/gpu/media/dxv... content/common/gpu/media/dxva_video_decode_accelerator.cc:21: base::LazyInstance<base::win::ScopedCOMInitializer> On 2011/11/14 20:55:07, apatrick_chromium wrote: > This happens in gpu_main.cc so it might not be necessary here. Also, I think > this might need to happen before the sandbox is enabled, so if it didn't happen > in gpu_main.cc, this might be too late. Removed. http://codereview.chromium.org/8510039/diff/3003/content/common/gpu/media/dxv... content/common/gpu/media/dxva_video_decode_accelerator.cc:201: ::OpenProcess(PROCESS_DUP_HANDLE, On 2011/11/14 20:55:07, apatrick_chromium wrote: > I think the sandbox might prevent OpenProcess from working. GpuChannel has a > handle for the corresponding renderer process though. Done. http://codereview.chromium.org/8510039/diff/3003/content/common/gpu/media/dxv... content/common/gpu/media/dxva_video_decode_accelerator.cc:408: HRESULT hr = CoCreateInstance(__uuidof(CMSH264DecoderMFT), On 2011/11/14 20:55:07, apatrick_chromium wrote: > Does this work when the sandbox is enabled? Yes. http://codereview.chromium.org/8510039/diff/3003/content/common/gpu/media/dxv... content/common/gpu/media/dxva_video_decode_accelerator.cc:708: EGLImageKHR egl_image = texture2eglImage_translator.TranslateToEglImage( On 2011/11/16 22:57:17, apatrick_chromium wrote: > ... and here we have a D3D surface (not a texture) that we need to copy into a > GL (ANGLE) texture by texture ID. This code doesn't work yet BTW; I think it was > copied from a platform that actually supports EGLImageKHR. Removed this. Was an incorrect cut paste.
Also, I think the first lint error is legit. http://codereview.chromium.org/8510039/diff/17001/content/common/gpu/media/dx... File content/common/gpu/media/dxva_video_decode_accelerator.cc (right): http://codereview.chromium.org/8510039/diff/17001/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:372: d3d9_.Attach(Direct3DCreate9(D3D_SDK_VERSION)); I don't think the IDirect3D9 needs to outlive the scope of this function. http://codereview.chromium.org/8510039/diff/17001/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:379: present_params.BackBufferWidth = 0; 0 by 0 might not work reliably. maybe 1 by 1 would be safer. http://codereview.chromium.org/8510039/diff/17001/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:381: present_params.BackBufferFormat = D3DFMT_UNKNOWN; and i'd use a valid format as well. http://codereview.chromium.org/8510039/diff/17001/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:390: HRESULT hr = d3d9_->CreateDevice(D3DADAPTER_DEFAULT, This will need to be a D3D9Ex device if we are going to share textures with ANGLE. http://codereview.chromium.org/8510039/diff/17001/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:393: (D3DCREATE_HARDWARE_VERTEXPROCESSING | Older Intel cards do not support hardware vertex processing. http://codereview.chromium.org/8510039/diff/17001/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:403: hr = DXVA2CreateDirect3DDeviceManager9(&dev_manager_reset_token, I think D3D retains a pointer to this and modifies its value every time the device is reset. This could corrupt the stack. http://codereview.chromium.org/8510039/diff/17001/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:510: IMFMediaType* out_media_type = NULL; nit: Why can't this be ScopedComPtr? http://codereview.chromium.org/8510039/diff/17001/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:546: // The MFT will not allocate buffer for neither input nor output, so we have nit: double negative http://codereview.chromium.org/8510039/diff/17001/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:555: LOG(INFO) << "Input stream info: "; would VLOG be more appropriate? http://codereview.chromium.org/8510039/diff/17001/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:589: // scoped_refptr<VideoFrame> frame; nit: is this old code that can be deleted? http://codereview.chromium.org/8510039/diff/17001/content/common/gpu/media/dx... File content/common/gpu/media/dxva_video_decode_accelerator.h (right): http://codereview.chromium.org/8510039/diff/17001/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.h:38: explicit DXVAVideoDecodeAccelerator( nit: explicit is redundant http://codereview.chromium.org/8510039/diff/17001/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.h:70: MessageLoop* message_loop_; MessageLoopProxy might be safer here if there is any danger of instances outliving the referenced message loop. http://codereview.chromium.org/8510039/diff/24001/content/common/gpu/media/dx... File content/common/gpu/media/dxva_video_decode_accelerator.cc (right): http://codereview.chromium.org/8510039/diff/24001/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:798: glBindTexture(GL_TEXTURE_2D, 0); You should restore the texture binding to whatever it was prior to the first call the glBindTexture if this is using the same context as the GLES2 decoder.
Exciting times! Reviewed everything but the meat of the change (the DXVA decoder) :) Will do that tonight or tomorrow morning. <time passes> Oh, I just realized you didn't send this out for real review yet. I guess I'll hold off on that until I hear otherwise from you. http://codereview.chromium.org/8510039/diff/24001/content/common/gpu/media/dx... File content/common/gpu/media/dxva_video_decode_accelerator.cc (right): http://codereview.chromium.org/8510039/diff/24001/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:21: #include "third_party/angle/include/EGL/egl.h" minimize headers? http://codereview.chromium.org/8510039/diff/24001/content/common/gpu/media/gp... File content/common/gpu/media/gpu_video_decode_accelerator.cc (right): http://codereview.chromium.org/8510039/diff/24001/content/common/gpu/media/gp... content/common/gpu/media/gpu_video_decode_accelerator.cc:127: init_done_msg_ = init_done_msg; This needs to be done outside any of the #if guards to ensure correct error return in case of unsupported HW. You should get that for free when merging in http://codereview.chromium.org/8922010/ (which should hopefully land & stick tonight). http://codereview.chromium.org/8510039/diff/24001/content/common/gpu/media/gp... File content/common/gpu/media/gpu_video_decode_accelerator.h (right): http://codereview.chromium.org/8510039/diff/24001/content/common/gpu/media/gp... content/common/gpu/media/gpu_video_decode_accelerator.h:50: base::ProcessHandle renderer_process); Document lifecycle expectations on this handle. http://codereview.chromium.org/8510039/diff/24001/content/content_common.gypi File content/content_common.gypi (right): http://codereview.chromium.org/8510039/diff/24001/content/content_common.gypi... content/content_common.gypi:330: '../third_party/angle/src/build_angle.gyp:libEGL', Are EGL/GLES2 really used by your impl? http://codereview.chromium.org/8510039/diff/24001/content/content_common.gypi... content/content_common.gypi:354: 'common/gpu/media/gles2_texture_to_egl_image_translator.cc', Drop gles2->EGL translator? http://codereview.chromium.org/8510039/diff/24001/content/renderer/gpu/gpu_vi... File content/renderer/gpu/gpu_video_decode_accelerator_host.cc (right): http://codereview.chromium.org/8510039/diff/24001/content/renderer/gpu/gpu_vi... content/renderer/gpu/gpu_video_decode_accelerator_host.cc:10: #include "base/process_util.h" Revert?
http://codereview.chromium.org/8510039/diff/17001/content/common/gpu/media/dx... File content/common/gpu/media/dxva_video_decode_accelerator.cc (right): http://codereview.chromium.org/8510039/diff/17001/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:372: d3d9_.Attach(Direct3DCreate9(D3D_SDK_VERSION)); On 2011/12/13 02:00:13, apatrick_chromium wrote: > I don't think the IDirect3D9 needs to outlive the scope of this function. Done. http://codereview.chromium.org/8510039/diff/17001/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:379: present_params.BackBufferWidth = 0; On 2011/12/13 02:00:13, apatrick_chromium wrote: > 0 by 0 might not work reliably. maybe 1 by 1 would be safer. Done. http://codereview.chromium.org/8510039/diff/17001/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:381: present_params.BackBufferFormat = D3DFMT_UNKNOWN; On 2011/12/13 02:00:13, apatrick_chromium wrote: > and i'd use a valid format as well. As per msdn D3DFMT_UNKNOWN can be specified for the BackBufferFormat while in windowed mode. This tells the runtime to use the current display-mode format and eliminates the need to call GetDisplayMode. http://codereview.chromium.org/8510039/diff/17001/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:390: HRESULT hr = d3d9_->CreateDevice(D3DADAPTER_DEFAULT, On 2011/12/13 02:00:13, apatrick_chromium wrote: > This will need to be a D3D9Ex device if we are going to share textures with > ANGLE. Done. http://codereview.chromium.org/8510039/diff/17001/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:393: (D3DCREATE_HARDWARE_VERTEXPROCESSING | On 2011/12/13 02:00:13, apatrick_chromium wrote: > Older Intel cards do not support hardware vertex processing. Changed to mixed. http://codereview.chromium.org/8510039/diff/17001/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:403: hr = DXVA2CreateDirect3DDeviceManager9(&dev_manager_reset_token, On 2011/12/13 02:00:13, apatrick_chromium wrote: > I think D3D retains a pointer to this and modifies its value every time the > device is reset. This could corrupt the stack. Done. http://codereview.chromium.org/8510039/diff/17001/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:510: IMFMediaType* out_media_type = NULL; On 2011/12/13 02:00:13, apatrick_chromium wrote: > nit: Why can't this be ScopedComPtr? Done. http://codereview.chromium.org/8510039/diff/17001/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:546: // The MFT will not allocate buffer for neither input nor output, so we have On 2011/12/13 02:00:13, apatrick_chromium wrote: > nit: double negative Done. http://codereview.chromium.org/8510039/diff/17001/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:555: LOG(INFO) << "Input stream info: "; On 2011/12/13 02:00:13, apatrick_chromium wrote: > would VLOG be more appropriate? Done. http://codereview.chromium.org/8510039/diff/17001/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:589: // scoped_refptr<VideoFrame> frame; On 2011/12/13 02:00:13, apatrick_chromium wrote: > nit: is this old code that can be deleted? Done. http://codereview.chromium.org/8510039/diff/17001/content/common/gpu/media/dx... File content/common/gpu/media/dxva_video_decode_accelerator.h (right): http://codereview.chromium.org/8510039/diff/17001/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.h:70: MessageLoop* message_loop_; On 2011/12/13 02:00:13, apatrick_chromium wrote: > MessageLoopProxy might be safer here if there is any danger of instances > outliving the referenced message loop. Done. http://codereview.chromium.org/8510039/diff/24001/content/common/gpu/media/dx... File content/common/gpu/media/dxva_video_decode_accelerator.cc (right): http://codereview.chromium.org/8510039/diff/24001/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:21: #include "third_party/angle/include/EGL/egl.h" On 2011/12/13 02:37:12, Ami Fischman wrote: > minimize headers? Done. http://codereview.chromium.org/8510039/diff/24001/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:798: glBindTexture(GL_TEXTURE_2D, 0); On 2011/12/13 02:00:13, apatrick_chromium wrote: > You should restore the texture binding to whatever it was prior to the first > call the glBindTexture if this is using the same context as the GLES2 decoder. Dunno how to restore the binding. Will look into that. http://codereview.chromium.org/8510039/diff/24001/content/common/gpu/media/gp... File content/common/gpu/media/gpu_video_decode_accelerator.cc (right): http://codereview.chromium.org/8510039/diff/24001/content/common/gpu/media/gp... content/common/gpu/media/gpu_video_decode_accelerator.cc:127: init_done_msg_ = init_done_msg; On 2011/12/13 02:37:12, Ami Fischman wrote: > This needs to be done outside any of the #if guards to ensure correct error > return in case of unsupported HW. You should get that for free when merging in > http://codereview.chromium.org/8922010/ (which should hopefully land & stick > tonight). Will merge with your change. http://codereview.chromium.org/8510039/diff/24001/content/content_common.gypi File content/content_common.gypi (right): http://codereview.chromium.org/8510039/diff/24001/content/content_common.gypi... content/content_common.gypi:330: '../third_party/angle/src/build_angle.gyp:libEGL', On 2011/12/13 02:37:12, Ami Fischman wrote: > Are EGL/GLES2 really used by your impl? Not currently. But will be once we have an EGL extension taking in the D3D surface for rendering. http://codereview.chromium.org/8510039/diff/24001/content/content_common.gypi... content/content_common.gypi:354: 'common/gpu/media/gles2_texture_to_egl_image_translator.cc', On 2011/12/13 02:37:12, Ami Fischman wrote: > Drop gles2->EGL translator? Done. http://codereview.chromium.org/8510039/diff/24001/content/renderer/gpu/gpu_vi... File content/renderer/gpu/gpu_video_decode_accelerator_host.cc (right): http://codereview.chromium.org/8510039/diff/24001/content/renderer/gpu/gpu_vi... content/renderer/gpu/gpu_video_decode_accelerator_host.cc:10: #include "base/process_util.h" On 2011/12/13 02:37:12, Ami Fischman wrote: > Revert? Done.
http://codereview.chromium.org/8510039/diff/24001/content/common/gpu/media/gp... File content/common/gpu/media/gpu_video_decode_accelerator.h (right): http://codereview.chromium.org/8510039/diff/24001/content/common/gpu/media/gp... content/common/gpu/media/gpu_video_decode_accelerator.h:50: base::ProcessHandle renderer_process); On 2011/12/13 02:37:12, Ami Fischman wrote: > Document lifecycle expectations on this handle. Done.
Reviewed the .h file & got to the bottom of Decode() in the .cc file. Will finish the .cc file tomorrow. http://codereview.chromium.org/8510039/diff/24001/content/content_common.gypi File content/content_common.gypi (right): http://codereview.chromium.org/8510039/diff/24001/content/content_common.gypi... content/content_common.gypi:330: '../third_party/angle/src/build_angle.gyp:libEGL', On 2011/12/13 02:51:18, ananta wrote: > On 2011/12/13 02:37:12, Ami Fischman wrote: > > Are EGL/GLES2 really used by your impl? > > Not currently. But will be once we have an EGL extension taking in the D3D > surface for rendering. ISTM best to drop the deps until they're needed, but do as you like. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... File content/common/gpu/media/dxva_video_decode_accelerator.cc (right): http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:4: Assert this file is only built on OS_WIN: #if !defined(OS_WIN) #error Oops #endif ? (I thought I had something similar in OVDA but I guess not) http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:14: #include "base/lazy_instance.h" remove http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:18: #include "base/message_loop.h" can remove if you take my NonThreadSafe suggestion http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:25: namespace { Since almost all contents of this are static and anonymous namespaces add tooling friction, I'd remove it. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:28: static const int kNumInputs = 100; Are these magic DXVA constants? Where do they come from? Add comments? http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:30: IMFSample* CreateEmptySample() { static http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:34: if (FAILED(hr)) { This seems like a pretty common pattern (similar to OVDA). You might want to do something like http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/content/common/gpu/me... http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:43: // If |align| is 0, then no alignment is specified. drop this line? http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:77: // the decoder for input). The times here should be given in 100ns units. Units of 100ns is pretty unchrome-y. Is it not a good idea to use TimeDelta here? http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:79: // alignment is required, provide 0 or 1. "0 or 1"? Any reason to allow/suggest both? http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:113: DWORD max_length = 0, current_length = 0; comma is unchrome-y. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:148: output_stream_info_(), This and the previous line are unnecessary, I think. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:169: NOTREACHED() << "Initialize: invalid state: " One line. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:176: NOTREACHED() << "MFStartup failed. Error:" Single line (here and below; I'm going to stop commenting on that). http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:186: if (SendMFTMessage(MFT_MESSAGE_NOTIFY_BEGIN_STREAMING, 0)) { If you reverse the test (!) you get to: - drop the braces - be symmetric w/ the other early-returns above. - de-indent 3 lines in exchange for indenting 1 line. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:198: if (state_ == DXVAVideoDecodeAccelerator::kUninitialized) { What about the other states (resetting/flushing)? You might want to do the ovda_test generification sooner rather than later (it tests a bunch of teardown scenarios, among other things). http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:204: if (!::DuplicateHandle(renderer_process_, Is this really necessary? I expected the fact that we tell the IPC machinery that the arg is a shm handle would mean it would get dup'd into this process automatically: http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/content/common/gpu/gp... If this isn't necessary, then I think you can drop process_handle_ from this class. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:226: exploded.second * 10000000, Wait, what? This is putting Now() into the decoder??? I assumed that param to CreateInputSample was about the video-stream timestamp, not the wall-clock time. What is this about? http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:227: 400000, Where does 400k come from? http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:239: SendMFTMessage(MFT_MESSAGE_NOTIFY_START_OF_STREAM, 0); No error-checking necessary? http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:250: state_ = DXVAVideoDecodeAccelerator::kEosDrain; You're draining the decoder after each sample? That seems wrong. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:255: client_->NotifyEndOfBitstreamBuffer(bitstream_buffer.id()); This is supposed to be called when the buffer has been fully decoded, not when it's been handed to the decoder. Is something above here synchronous? (it's a problem either way) http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:527: reinterpret_cast<UINT32*>(&surface_height_)); surface_{width,height}_ don't seem to be used anywhere. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... File content/common/gpu/media/dxva_video_decode_accelerator.h (right): http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.h:1: // Copyright (c) 2011 The Chromium Authors. All rights reserved. FYI the CL description describes omx_v_d_a_test as being a browsertest, but it's not - it's a standalone gtest target. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.h:5: #ifndef CONTENT_COMMON_GPU_MEDIA_DVX_VIDEO_DECODE_ACCELERATOR_H_ s/DVX/DXVA/ (here and in the other 2 related lines) http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.h:29: typedef enum { FWIW this is C++; you can say enum State { ... }; which seems much more common in chromium code. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.h:38: explicit DXVAVideoDecodeAccelerator( explicit unneeded (unless you drop renderer_process as suggested in another comment). http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.h:44: bool Initialize(Profile profile) OVERRIDE; OVERRIDE requires 'virtual' (or else the clang bots explode) http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.h:54: bool CreateD3DDevManager(); There's a real dearth of commentary here. I think most/all of these have a single call-site and you extracted them b/c they look big with all the error-handling. Hopefully my suggestion about macrofying error handling (see .cc file) will make these inline-able at their (unique) call-sites. Otherwise please provide some commentary for what these are about. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.h:69: private: You already said private: at l.53 above. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.h:70: base::MessageLoopProxy* message_loop_; I think this is only used to assert being on the correct thread, in which case base::NonThreadSafe might be more idiomatic. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.h:72: // NOTE: all calls to this object *MUST* be executed in message_loop_. Can drop comment this if the class becomes NonThreadSafe. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.h:94: typedef std::vector<PendingSampleInfo> Single line http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.h:104: : picture_buffer(0, gfx::Size(), 0) { ctor definition belongs out-of-line in the .cc file. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.h:105: available = false; Why not initializer-list this one too? http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.h:111: typedef std::map<int32, DXVAPictureBuffer> OutputBuffers; doco the key. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.h:114: // Set to true if we requested picture slots from the plugin. s/plugin/client/ http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.h:114: // Set to true if we requested picture slots from the plugin. s/slots/buffers/ http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.h:117: // Contains the list of input buffer ids which map to frame time from the I don't understand this comment (or how the vector is used in the .cc file). I *think* this wants to be: // The i'th entry in this vector is the input buffer's ID currently occupying input slot i of the decoder. But I'm having a hard time wrapping my head around how the code in the .cc file is a correct implementation of this. Perhaps you're doing something completely different? http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.h:124: uint32 dev_manager_reset_token_; Can this not be a local variable in the (only) function it's used? http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.h:127: uint32 decode_start_time_; This smells like something that must have a generic, efficient, implementation, possible in a macro involving the word TRACE: http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/base/debug/trace_even... Does none of that work for you? (using trace_event.h buys you all kinds of snazzy visualization and analysis for free, plus avoids this sort of home-brew) http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.h:130: }; DISALLOW_?
Done. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... File content/common/gpu/media/dxva_video_decode_accelerator.cc (right): http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:165: bool DXVAVideoDecodeAccelerator::Initialize(Profile profile) { profile is unused in this file. Does the MF decoder not have that as an input param somewhere? http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:264: DXVAPictureBuffer picture_buffer; Use the ctor instead (but see below and just drop these 3 lines instead). http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:270: available_pictures_[buffers[buffer_index].id()] = picture_buffer; A more efficient way is to replace the above 3 lines with: bool inserted = available_pictures_.insert(std::make_pair( buffers[buffer_index].id(), DXVAPictureBuffer(true, buffers[buffer_index])).second; DCHECK(inserted); (more efficient b/c it only does the lookup once instead of twice (once for the find and once for the operator[])) http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:275: HRESULT hr = E_FAIL; None of the above 4 lines are used. I would have expected the cros gcc (4.6) to error out on these unused variables. Do you know why it didn't? http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:284: DCHECK(available_pictures_.find(picture_buffer_id) != FWIW, instead of find() != end(), it's shorter to DCHECK_EQ(my_map.count(key), 0U); (or just DCHECK(!my_map.count(key)); if you're feeling hardcore) http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:286: available_pictures_[picture_buffer_id].available = true; Since you end up needing this several more times in this function, I'd replace the above three lines with: OutputBuffers::iterator it = available_pictures_.find(picture_buffer_id); RETURN_ON_ERROR(it == available_pictures_.end()); it->available = true; and then later on use *it or it-> instead of repeating the lookup available_pictures_[picture_buffer_id] (or even store *it in an explicit DXVAPictureBuffer*). http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:288: PendingOutputSamples::iterator sample_index = This would be clearer as if (pending_output_samples_.empty()) return; const PendingSampleInfo& sample_info = *pending_output_samples_.front(); But can't this entire stanza be replaced with a call to ProcessPendingSamples? As it stands this code is very similar to that code so it'd be nice to avoid the duplication. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:298: pending_output_samples_.erase(sample_index); pop_front if you take my suggestion above (and I'd pop it immediately after taking the front() above). http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:305: VLOG(1) << "DXVAVideoDecodeAccelerator::Flush"; FWIW s/VLOG/DVLOG/g everywhere in this CL (see chromium-dev discussions started by brettw a few weeks ago). http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:308: client_->NotifyFlushDone(); It is generally not OK to call PPP_* methods from the implementation of PPB_* methods (b/c ppapi is not generally reentrant). Here (and in many places below) you're effectively relying on the fact that the enclosing message is async (so the PPB_* call has already returned by the time this is executing). I *think* this is OK, but wanted to point it out so you can think about it explicitly if you hadn't already done so. (audit all client_-> sites in this file). http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:308: client_->NotifyFlushDone(); Shouldn't this be a NotifyError(ILLEGAL_STATE)? In fact the presence of so much error checking with zero calls to NotifyError is suspicious; audit the failure paths? (again reco to use macros for failure-handling to reduce boilerplate). http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:313: VLOG(1) << "No data available from the decoder"; This doesn't seem to be worthy of a VLOG to me. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:318: state_ = DXVAVideoDecodeAccelerator::kEosDrain; qualification unnecessary (here and elsewhere below). http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:320: LOG(WARNING) << "Failed to send drain message"; Consistency with other MF failures. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:326: // As per MSDN docs after the client sends this message, it calls s/this/the above MFT_MESSAGE_COMMAND_DRAIN/ ? http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:332: DoDecode(); I guess DoDecode() really is synchronous. Are you not worried about blocking the GPU process for all other operation while this is happening? If the decoder is fed a bunch of input (without draining the output) and then fed a flush, and then outputs are drained (pictures are reused), can the page scroll at all? http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:347: state_ = DXVAVideoDecodeAccelerator::kResetting; Whenever you have code like this that checks that state is one of a set number of errors, else set to the next state, it's nice to be able to DCHECK the state *before* the assignment to document your intent/expectations. (here and elsewhere in this CL) http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:354: client_->NotifyResetDone(); This should be the last call in the method. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:355: input_buffer_frame_times_.clear(); What about the rest of the pending state? http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:369: } Does nothing need to be done to tear down the decoder? Destroy() should be the opposite of Initialize(). http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:512: bool found = false; unused http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:513: while (SUCCEEDED(decoder_->GetOutputAvailableType( ISTM this would be more natural as a for-loop for (DWORD i = 0; SUCCEEDED(decoder_->GetOutputAvailableType(0, i, out_media_type.Receive()); ++i) { ... http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:520: out_media_type.Release(); Unnecessary? (here and elsewhere) http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:524: hr = decoder_->SetOutputType(0, out_media_type, 0); // No flags No checking for failures? http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:550: // Exception is when dxva is enabled, the decoder will allocate output. What's the alternative? Can dxva be "disabled" and this class still do useful work? Apparently not, given the CHECK 0x107u below. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:557: VLOG(1) << "Input stream info: "; Between the VLOGs and the error-handling it's really hard to see that this 33-line function makes only 2 function calls that aren't error-checking/vlogging. I hope you can improve that ratio. (this is a generic comment about this file) http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:563: << std::hex << std::showbase << input_stream_info_.dwFlags; Unnecessary since the CHECK_EQ in the next line will tell us if it isn't 0x7u (and tell us what it is). http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:595: memset(&output_data_buffer, 0, sizeof(output_data_buffer)); This is incorrect in general; prefer the = {0}; style for zero-initialization. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:597: status = 0; redundant. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:610: hr = SetDecoderOutputMediaType(MFVideoFormat_NV12); I don't understand this - the stream changed nothing about the change is reflected in this call? http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:618: // No more output from the decoder. Notify EOS and stop playback. DCHECK that we were waiting for this? http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:618: // No more output from the decoder. Notify EOS and stop playback. This doesn't actually Notify or stop, it just signals that that should happen to the caller. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:638: state_ = DXVAVideoDecodeAccelerator::kStopped; More like error than stopped. NotifyError? http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:643: state_ = DXVAVideoDecodeAccelerator::kNormal; Why overwrite the state if we were stopping? http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:644: return true; Return value is ignored at all call-sites. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:656: VLOG(1) << "In input buffers left to process output"; what? http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:660: input_buffer_frame_times_[input_buffer_frame_times_.size() - 1]; I extra don't understand input_buffer_frame_times_ considering all the decode is synchronous and there doesn't seem to be queuing of inputs; why can't you just remember the "current" input buffer id? http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:688: // TODO(ananta) TODOs are usually marked w/ an email address. s/ananta/iyengar/? http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:721: PendingSampleInfo sample_info; Give PSI a ctor and use it here? In fact can replace l.721-725 with pending_output_samples_.push_back(PendingSampleInfo(input_buffer_id, dest_surface); http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:752: drop \n http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:765: // TODO(ananta) ditto http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:772: GetObject(bitmap, sizeof(BITMAP), &bitmap_basic_info); failure handling http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:784: int ret = GetDIBits(hdc, bitmap, 0, 0, NULL, &bitmap_info, DIB_RGB_COLORS); failure handling http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:789: scoped_ptr<char> bits(new char[bitmap_info.bmiHeader.biSizeImage]); scoped_array http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:792: DCHECK_GT(ret, 0); You seem to have given up on error handling from here down ;) http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:814: HRESULT hr = E_FAIL; Unused. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:816: while (sample_index != pending_output_samples_.end()) { This while smells like a for, but more than that the inner for is going to re-iterate the unavailable pictures for each pending output sample, which seems suboptimal. It would be much better to iterate over available_pictures_ once, front()/pop_front()'ing pending_output_samples_ each time you see an available picture, and returning when out of pending_output_samples_. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... File content/common/gpu/media/dxva_video_decode_accelerator.h (right): http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.h:23: // Class to provide a DXVA 2.0 based accelerator behind VideoDecodeAccelerator Is this really DXVA2.0 or is it MF? I know *nothing* about either of these APIs but ISTM that most/all of your platform calls are MF. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.h:112: OutputBuffers available_pictures_; This is a poor name considering some of its elements are not in fact available.
http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... File content/common/gpu/media/dxva_video_decode_accelerator.cc (right): http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:177: << std::hex << std::showbase << hr; is the object behind NOTREACHED an actual iostream? in other words do std::hex and std::showbase do anything? http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:741: pictures_requested_ = true; so pictures_requested_ does not become false again?
BTW I should have mentioned in my review explicitly: I don't know the MF APIs at all and hope someone else can review their usage for correctness. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... File content/common/gpu/media/dxva_video_decode_accelerator.cc (right): http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:177: << std::hex << std::showbase << hr; On 2011/12/13 19:28:57, cpu wrote: > is the object behind NOTREACHED an actual iostream? in other words do std::hex > and std::showbase do anything? Yes; NOTREACHED is (always) a DCHECK(false) under the covers.
http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... File content/common/gpu/media/dxva_video_decode_accelerator.cc (right): http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:204: if (!::DuplicateHandle(renderer_process_, On 2011/12/13 07:24:56, Ami Fischman wrote: > Is this really necessary? I expected the fact that we tell the IPC machinery > that the arg is a shm handle would mean it would get dup'd into this process > automatically: > http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/content/common/gpu/gp... > > If this isn't necessary, then I think you can drop process_handle_ from this > class. Last time I checked, the IPC system didn't know how to automatically dup shared memory handles on Windows. I think this is necessary. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:374: Direct3DCreate9Ex(D3D_SDK_VERSION, d3d9.Receive()); If the delay loading of d3d9.dll doesn't work for whatever reason (and the gyp file looks right), the XP bots will explode when you land this on account of not being able to bind to this import. Good luck! (I gave up and just GetProcAddressed it). http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:386: present_params.hDeviceWindow = GetShellWindow(); Question for cpu: what will GetShellWindow() do in a world where the GPU process is not running on the interactive desktop? http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:395: D3DCREATE_MIXED_VERTEXPROCESSING, I think MIXED won't work on cards that do not support hardware vertex shaders either. Since you aren't using vertex shaders anyway, you could just use SOFTWARE. Otherwise, the usual pattern is to ask for HARDWARE and if CreateDeviceEx fails, ask for SOFTWARE.
http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... File content/common/gpu/media/dxva_video_decode_accelerator.cc (right): http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:688: // TODO(ananta) On 2011/12/13 16:44:06, Ami Fischman wrote: > TODOs are usually marked w/ an email address. s/ananta/iyengar/? oops, I see that's your @chromium address. Disregard.
http://codereview.chromium.org/8510039/diff/24001/content/common/gpu/media/dx... File content/common/gpu/media/dxva_video_decode_accelerator.cc (right): http://codereview.chromium.org/8510039/diff/24001/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:798: glBindTexture(GL_TEXTURE_2D, 0); On 2011/12/13 02:51:18, ananta wrote: > On 2011/12/13 02:00:13, apatrick_chromium wrote: > > You should restore the texture binding to whatever it was prior to the first > > call the glBindTexture if this is using the same context as the GLES2 decoder. > > Dunno how to restore the binding. Will look into that. Done. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... File content/common/gpu/media/dxva_video_decode_accelerator.cc (right): http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:4: On 2011/12/13 07:24:56, Ami Fischman wrote: > Assert this file is only built on OS_WIN: > #if !defined(OS_WIN) > #error Oops > #endif > ? > > (I thought I had something similar in OVDA but I guess not) Done. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:4: On 2011/12/13 07:24:56, Ami Fischman wrote: > Assert this file is only built on OS_WIN: > #if !defined(OS_WIN) > #error Oops > #endif > ? > > (I thought I had something similar in OVDA but I guess not) Done. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:14: #include "base/lazy_instance.h" On 2011/12/13 07:24:56, Ami Fischman wrote: > remove Done. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:14: #include "base/lazy_instance.h" On 2011/12/13 07:24:56, Ami Fischman wrote: > remove Done. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:18: #include "base/message_loop.h" On 2011/12/13 07:24:56, Ami Fischman wrote: > can remove if you take my NonThreadSafe suggestion Done. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:18: #include "base/message_loop.h" On 2011/12/13 07:24:56, Ami Fischman wrote: > can remove if you take my NonThreadSafe suggestion Done. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:25: namespace { On 2011/12/13 07:24:56, Ami Fischman wrote: > Since almost all contents of this are static and anonymous namespaces add > tooling friction, I'd remove it. Done. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:25: namespace { On 2011/12/13 07:24:56, Ami Fischman wrote: > Since almost all contents of this are static and anonymous namespaces add > tooling friction, I'd remove it. Done. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:30: IMFSample* CreateEmptySample() { On 2011/12/13 07:24:56, Ami Fischman wrote: > static Done. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:34: if (FAILED(hr)) { On 2011/12/13 07:24:56, Ami Fischman wrote: > This seems like a pretty common pattern (similar to OVDA). > You might want to do something like > http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/content/common/gpu/me... Done. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:43: // If |align| is 0, then no alignment is specified. On 2011/12/13 07:24:56, Ami Fischman wrote: > drop this line? Done. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:43: // If |align| is 0, then no alignment is specified. On 2011/12/13 07:24:56, Ami Fischman wrote: > drop this line? Done. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:77: // the decoder for input). The times here should be given in 100ns units. On 2011/12/13 07:24:56, Ami Fischman wrote: > Units of 100ns is pretty unchrome-y. > Is it not a good idea to use TimeDelta here? Unnecessary now. Removed these parameters as we don't receive them from the plugin. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:77: // the decoder for input). The times here should be given in 100ns units. On 2011/12/13 07:24:56, Ami Fischman wrote: > Units of 100ns is pretty unchrome-y. > Is it not a good idea to use TimeDelta here? Removed these parameters as they are not needed. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:79: // alignment is required, provide 0 or 1. On 2011/12/13 07:24:56, Ami Fischman wrote: > "0 or 1"? Any reason to allow/suggest both? Bad comment. Fixed. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:79: // alignment is required, provide 0 or 1. On 2011/12/13 07:24:56, Ami Fischman wrote: > "0 or 1"? Any reason to allow/suggest both? Fixed http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:113: DWORD max_length = 0, current_length = 0; On 2011/12/13 07:24:56, Ami Fischman wrote: > comma is unchrome-y. Done. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:113: DWORD max_length = 0, current_length = 0; On 2011/12/13 07:24:56, Ami Fischman wrote: > comma is unchrome-y. Done. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:148: output_stream_info_(), On 2011/12/13 07:24:56, Ami Fischman wrote: > This and the previous line are unnecessary, I think. Done. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:148: output_stream_info_(), On 2011/12/13 07:24:56, Ami Fischman wrote: > This and the previous line are unnecessary, I think. Done. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:165: bool DXVAVideoDecodeAccelerator::Initialize(Profile profile) { On 2011/12/13 16:44:06, Ami Fischman wrote: > profile is unused in this file. Does the MF decoder not have that as an input > param somewhere? We don't need to pass it specifically. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:169: NOTREACHED() << "Initialize: invalid state: " On 2011/12/13 07:24:56, Ami Fischman wrote: > One line. Done. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:176: NOTREACHED() << "MFStartup failed. Error:" On 2011/12/13 07:24:56, Ami Fischman wrote: > Single line (here and below; I'm going to stop commenting on that). Done. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:186: if (SendMFTMessage(MFT_MESSAGE_NOTIFY_BEGIN_STREAMING, 0)) { On 2011/12/13 07:24:56, Ami Fischman wrote: > If you reverse the test (!) you get to: > - drop the braces > - be symmetric w/ the other early-returns above. > - de-indent 3 lines in exchange for indenting 1 line. Done. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:198: if (state_ == DXVAVideoDecodeAccelerator::kUninitialized) { On 2011/12/13 07:24:56, Ami Fischman wrote: > What about the other states (resetting/flushing)? > You might want to do the ovda_test generification sooner rather than later (it > tests a bunch of teardown scenarios, among other things). Will do that once this change clears up. Fixed this check to just allow decoding in the normal and stopped state. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:204: if (!::DuplicateHandle(renderer_process_, On 2011/12/13 07:24:56, Ami Fischman wrote: > Is this really necessary? I expected the fact that we tell the IPC machinery > that the arg is a shm handle would mean it would get dup'd into this process > automatically: > http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/content/common/gpu/gp... > > If this isn't necessary, then I think you can drop process_handle_ from this > class. The shared memory handle is not duplicated into the receiving process by the IPC machinery. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:226: exploded.second * 10000000, On 2011/12/13 07:24:56, Ami Fischman wrote: > Wait, what? This is putting Now() into the decoder??? > I assumed that param to CreateInputSample was about the video-stream timestamp, > not the wall-clock time. What is this about? Removed. I had put that only for debugging i.e. to find out whether the buffering in the decoder was due to these missing attributes. Turned out to be wrong. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:227: 400000, On 2011/12/13 07:24:56, Ami Fischman wrote: > Where does 400k come from? From thin air. :) Removed this parameter. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:239: SendMFTMessage(MFT_MESSAGE_NOTIFY_START_OF_STREAM, 0); On 2011/12/13 07:24:56, Ami Fischman wrote: > No error-checking necessary? Done. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:250: state_ = DXVAVideoDecodeAccelerator::kEosDrain; On 2011/12/13 07:24:56, Ami Fischman wrote: > You're draining the decoder after each sample? That seems wrong. I am not draining the decoder. The state is marked as eosdrain. The command being sent to the decoder above is end of stream. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:255: client_->NotifyEndOfBitstreamBuffer(bitstream_buffer.id()); On 2011/12/13 07:24:56, Ami Fischman wrote: > This is supposed to be called when the buffer has been fully decoded, not when > it's been handed to the decoder. Is something above here synchronous? (it's a > problem either way) We already handed the decoder the buffer to be decoded. It makes a copy of the buffer internally. Unfortunately we need to inform the client that the input has been processed. (Read flash). If not it stops sending us input packets as it runs out of free buffers. The MFT decoder does do a bit of buffering before it gives as a valid decoded sample. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:264: DXVAPictureBuffer picture_buffer; On 2011/12/13 16:44:06, Ami Fischman wrote: > Use the ctor instead (but see below and just drop these 3 lines instead). Done. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:270: available_pictures_[buffers[buffer_index].id()] = picture_buffer; On 2011/12/13 16:44:06, Ami Fischman wrote: > A more efficient way is to replace the above 3 lines with: > bool inserted = available_pictures_.insert(std::make_pair( > buffers[buffer_index].id(), DXVAPictureBuffer(true, > buffers[buffer_index])).second; > DCHECK(inserted); > > (more efficient b/c it only does the lookup once instead of twice (once for the > find and once for the operator[])) The find above is just checking whether the picture id already exists in the map which should not happen. Leaving as is. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:275: HRESULT hr = E_FAIL; On 2011/12/13 16:44:06, Ami Fischman wrote: > None of the above 4 lines are used. > I would have expected the cros gcc (4.6) to error out on these unused variables. > Do you know why it didn't? Dunno why. These statements were left out by mistake while refactoring this function to move the common code out to ProcessPendingSamples. Removed these statements. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:284: DCHECK(available_pictures_.find(picture_buffer_id) != On 2011/12/13 16:44:06, Ami Fischman wrote: > FWIW, instead of find() != end(), it's shorter to > DCHECK_EQ(my_map.count(key), 0U); > (or just DCHECK(!my_map.count(key)); if you're feeling hardcore) Done. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:286: available_pictures_[picture_buffer_id].available = true; On 2011/12/13 16:44:06, Ami Fischman wrote: > Since you end up needing this several more times in this function, I'd replace > the above three lines with: > OutputBuffers::iterator it = available_pictures_.find(picture_buffer_id); > RETURN_ON_ERROR(it == available_pictures_.end()); > it->available = true; > > and then later on use *it or it-> instead of repeating the lookup > available_pictures_[picture_buffer_id] (or even store *it in an explicit > DXVAPictureBuffer*). Done. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:288: PendingOutputSamples::iterator sample_index = On 2011/12/13 16:44:06, Ami Fischman wrote: > This would be clearer as > if (pending_output_samples_.empty()) > return; > const PendingSampleInfo& sample_info = *pending_output_samples_.front(); > > But can't this entire stanza be replaced with a call to ProcessPendingSamples? > As it stands this code is very similar to that code so it'd be nice to avoid the > duplication. Done. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:298: pending_output_samples_.erase(sample_index); On 2011/12/13 16:44:06, Ami Fischman wrote: > pop_front if you take my suggestion above (and I'd pop it immediately after > taking the front() above). Ignoring this as I took your above suggestion to call ProcessPendingSamples instead. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:308: client_->NotifyFlushDone(); On 2011/12/13 16:44:06, Ami Fischman wrote: > Shouldn't this be a NotifyError(ILLEGAL_STATE)? > In fact the presence of so much error checking with zero calls to NotifyError is > suspicious; audit the failure paths? > (again reco to use macros for failure-handling to reduce boilerplate). Done. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:313: VLOG(1) << "No data available from the decoder"; On 2011/12/13 16:44:06, Ami Fischman wrote: > This doesn't seem to be worthy of a VLOG to me. Rewrote this function. PTAL. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:318: state_ = DXVAVideoDecodeAccelerator::kEosDrain; On 2011/12/13 16:44:06, Ami Fischman wrote: > qualification unnecessary (here and elsewhere below). Done. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:320: LOG(WARNING) << "Failed to send drain message"; On 2011/12/13 16:44:06, Ami Fischman wrote: > Consistency with other MF failures. This function has been rewritten to make it look better. PTAL. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:326: // As per MSDN docs after the client sends this message, it calls On 2011/12/13 16:44:06, Ami Fischman wrote: > s/this/the above MFT_MESSAGE_COMMAND_DRAIN/ ? Yes. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:332: DoDecode(); On 2011/12/13 16:44:06, Ami Fischman wrote: > I guess DoDecode() really is synchronous. Are you not worried about blocking > the GPU process for all other operation while this is happening? > If the decoder is fed a bunch of input (without draining the output) and then > fed a flush, and then outputs are drained (pictures are reused), can the page > scroll at all? Worried a little yes. However the MFT decoder uses a thread pool internally. Based on debugging I found that processoutput just checks the state of an event which is set when there is decoded output available. So hopefully this should not be much of an overhead. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:354: client_->NotifyResetDone(); On 2011/12/13 16:44:06, Ami Fischman wrote: > This should be the last call in the method. Done. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:355: input_buffer_frame_times_.clear(); On 2011/12/13 16:44:06, Ami Fischman wrote: > What about the rest of the pending state? Done. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:512: bool found = false; On 2011/12/13 16:44:06, Ami Fischman wrote: > unused Done. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:513: while (SUCCEEDED(decoder_->GetOutputAvailableType( On 2011/12/13 16:44:06, Ami Fischman wrote: > ISTM this would be more natural as a for-loop > for (DWORD i = 0; SUCCEEDED(decoder_->GetOutputAvailableType(0, i, > out_media_type.Receive()); ++i) { ... Done. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:520: out_media_type.Release(); On 2011/12/13 16:44:06, Ami Fischman wrote: > Unnecessary? (here and elsewhere) We have to release the COM interface here inorder to fetch the next guy. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:524: hr = decoder_->SetOutputType(0, out_media_type, 0); // No flags On 2011/12/13 16:44:06, Ami Fischman wrote: > No checking for failures? Done. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:527: reinterpret_cast<UINT32*>(&surface_height_)); On 2011/12/13 07:24:56, Ami Fischman wrote: > surface_{width,height}_ don't seem to be used anywhere. Done. PTAL at this function. Rewritten to abort on errors. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:550: // Exception is when dxva is enabled, the decoder will allocate output. On 2011/12/13 16:44:06, Ami Fischman wrote: > What's the alternative? Can dxva be "disabled" and this class still do useful > work? Apparently not, given the CHECK 0x107u below. Unfortunate cut paste. Rephrased the comment. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:557: VLOG(1) << "Input stream info: "; On 2011/12/13 16:44:06, Ami Fischman wrote: > Between the VLOGs and the error-handling it's really hard to see that this > 33-line function makes only 2 function calls that aren't > error-checking/vlogging. I hope you can improve that ratio. > > (this is a generic comment about this file) Fixed this function to get the input and output streams at the beginning. Will take a look at the rest of the functions. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:595: memset(&output_data_buffer, 0, sizeof(output_data_buffer)); On 2011/12/13 16:44:06, Ami Fischman wrote: > This is incorrect in general; prefer the = {0}; style for zero-initialization. Done. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:597: status = 0; On 2011/12/13 16:44:06, Ami Fischman wrote: > redundant. Done. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:610: hr = SetDecoderOutputMediaType(MFVideoFormat_NV12); On 2011/12/13 16:44:06, Ami Fischman wrote: > I don't understand this - the stream changed nothing about the change is > reflected in this call? This is how MSDN mentions that we should handle the format change :(. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:618: // No more output from the decoder. Notify EOS and stop playback. On 2011/12/13 16:44:06, Ami Fischman wrote: > This doesn't actually Notify or stop, it just signals that that should happen to > the caller. Could you clarify this?. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:638: state_ = DXVAVideoDecodeAccelerator::kStopped; On 2011/12/13 16:44:06, Ami Fischman wrote: > More like error than stopped. NotifyError? Done. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:643: state_ = DXVAVideoDecodeAccelerator::kNormal; On 2011/12/13 16:44:06, Ami Fischman wrote: > Why overwrite the state if we were stopping? When we arrive here we are not stopping as we have output to process. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:644: return true; On 2011/12/13 16:44:06, Ami Fischman wrote: > Return value is ignored at all call-sites. Done. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:656: VLOG(1) << "In input buffers left to process output"; On 2011/12/13 16:44:06, Ami Fischman wrote: > what? Replaced with No. :( http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:721: PendingSampleInfo sample_info; On 2011/12/13 16:44:06, Ami Fischman wrote: > Give PSI a ctor and use it here? > In fact can replace l.721-725 with > pending_output_samples_.push_back(PendingSampleInfo(input_buffer_id, > dest_surface); Done. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:741: pictures_requested_ = true; On 2011/12/13 19:28:57, cpu wrote: > so pictures_requested_ does not become false again? We only do this upfront. After that we just rotate among the set the client provided us. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:752: On 2011/12/13 16:44:06, Ami Fischman wrote: > drop \n Done. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:765: // TODO(ananta) On 2011/12/13 16:44:06, Ami Fischman wrote: > ditto Done. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:772: GetObject(bitmap, sizeof(BITMAP), &bitmap_basic_info); On 2011/12/13 16:44:06, Ami Fischman wrote: > failure handling Done. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:784: int ret = GetDIBits(hdc, bitmap, 0, 0, NULL, &bitmap_info, DIB_RGB_COLORS); On 2011/12/13 16:44:06, Ami Fischman wrote: > failure handling Done. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:789: scoped_ptr<char> bits(new char[bitmap_info.bmiHeader.biSizeImage]); On 2011/12/13 16:44:06, Ami Fischman wrote: > scoped_array Done. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:792: DCHECK_GT(ret, 0); On 2011/12/13 16:44:06, Ami Fischman wrote: > You seem to have given up on error handling from here down ;) Done. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:814: HRESULT hr = E_FAIL; On 2011/12/13 16:44:06, Ami Fischman wrote: > Unused. Done. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:816: while (sample_index != pending_output_samples_.end()) { On 2011/12/13 16:44:06, Ami Fischman wrote: > This while smells like a for, but more than that the inner for is going to > re-iterate the unavailable pictures for each pending output sample, which seems > suboptimal. It would be much better to iterate over available_pictures_ once, > front()/pop_front()'ing pending_output_samples_ each time you see an available > picture, and returning when out of pending_output_samples_. Done. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... File content/common/gpu/media/dxva_video_decode_accelerator.h (right): http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.h:5: #ifndef CONTENT_COMMON_GPU_MEDIA_DVX_VIDEO_DECODE_ACCELERATOR_H_ On 2011/12/13 07:24:56, Ami Fischman wrote: > s/DVX/DXVA/ > (here and in the other 2 related lines) Done. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.h:23: // Class to provide a DXVA 2.0 based accelerator behind VideoDecodeAccelerator On 2011/12/13 16:44:06, Ami Fischman wrote: > Is this really DXVA2.0 or is it MF? I know *nothing* about either of these APIs > but ISTM that most/all of your platform calls are MF. Fixed the comment. Thanks for pointing this out. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.h:29: typedef enum { On 2011/12/13 07:24:56, Ami Fischman wrote: > FWIW this is C++; you can say > enum State { > ... > }; > which seems much more common in chromium code. Done. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.h:38: explicit DXVAVideoDecodeAccelerator( On 2011/12/13 07:24:56, Ami Fischman wrote: > explicit unneeded (unless you drop renderer_process as suggested in another > comment). Done. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.h:44: bool Initialize(Profile profile) OVERRIDE; On 2011/12/13 07:24:56, Ami Fischman wrote: > OVERRIDE requires 'virtual' (or else the clang bots explode) Done. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.h:69: private: On 2011/12/13 07:24:56, Ami Fischman wrote: > You already said private: at l.53 above. I added this because this section has member variables. Removed now. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.h:70: base::MessageLoopProxy* message_loop_; On 2011/12/13 07:24:56, Ami Fischman wrote: > I think this is only used to assert being on the correct thread, in which case > base::NonThreadSafe might be more idiomatic. Done. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.h:72: // NOTE: all calls to this object *MUST* be executed in message_loop_. On 2011/12/13 07:24:56, Ami Fischman wrote: > Can drop comment this if the class becomes NonThreadSafe. Done. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.h:94: typedef std::vector<PendingSampleInfo> On 2011/12/13 07:24:56, Ami Fischman wrote: > Single line Done. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.h:105: available = false; On 2011/12/13 07:24:56, Ami Fischman wrote: > Why not initializer-list this one too? Done. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.h:111: typedef std::map<int32, DXVAPictureBuffer> OutputBuffers; On 2011/12/13 07:24:56, Ami Fischman wrote: > doco the key. Done. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.h:112: OutputBuffers available_pictures_; On 2011/12/13 16:44:06, Ami Fischman wrote: > This is a poor name considering some of its elements are not in fact available. Changed to output_picture_buffers_ http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.h:114: // Set to true if we requested picture slots from the plugin. On 2011/12/13 07:24:56, Ami Fischman wrote: > s/plugin/client/ Done. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.h:117: // Contains the list of input buffer ids which map to frame time from the On 2011/12/13 07:24:56, Ami Fischman wrote: > I don't understand this comment (or how the vector is used in the .cc file). > I *think* this wants to be: > // The i'th entry in this vector is the input buffer's ID currently occupying > input slot i of the decoder. > But I'm having a hard time wrapping my head around how the code in the .cc file > is a correct implementation of this. Perhaps you're doing something completely > different? Vector not needed as you mentioned in the cc file. We just maintain the last input buffer id. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.h:124: uint32 dev_manager_reset_token_; On 2011/12/13 07:24:56, Ami Fischman wrote: > Can this not be a local variable in the (only) function it's used? Please see Al Patrick's comment in the initial snapshot. It looks the decoder holds onto the pointer and tries to update it when the device info changes :( thus corrupting the stack. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.h:127: uint32 decode_start_time_; On 2011/12/13 07:24:56, Ami Fischman wrote: > This smells like something that must have a generic, efficient, implementation, > possible in a macro involving the word TRACE: > http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/base/debug/trace_even... > Does none of that work for you? > > (using trace_event.h buys you all kinds of snazzy visualization and analysis for > free, plus avoids this sort of home-brew) Will take a look at this. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.h:130: }; On 2011/12/13 07:24:56, Ami Fischman wrote: > DISALLOW_? Done.
http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... File content/common/gpu/media/dxva_video_decode_accelerator.h (right): http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.h:127: uint32 decode_start_time_; On 2011/12/13 07:24:56, Ami Fischman wrote: > This smells like something that must have a generic, efficient, implementation, > possible in a macro involving the word TRACE: > http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/base/debug/trace_even... > Does none of that work for you? > > (using trace_event.h buys you all kinds of snazzy visualization and analysis for > free, plus avoids this sort of home-brew) Fixed. We now use the ETW trace macros for Windows. TRACE_EVENT**
Mostly nits. http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... File content/common/gpu/media/dxva_video_decode_accelerator.h (right): http://codereview.chromium.org/8510039/diff/34002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.h:23: // Class to provide a DXVA 2.0 based accelerator behind VideoDecodeAccelerator On 2011/12/13 23:29:15, ananta wrote: > On 2011/12/13 16:44:06, Ami Fischman wrote: > > Is this really DXVA2.0 or is it MF? I know *nothing* about either of these > APIs > > but ISTM that most/all of your platform calls are MF. > > Fixed the comment. Thanks for pointing this out. > > So you're saying this really is DXVA2.0-specific, not just MF? http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/dx... File content/common/gpu/media/dxva_video_decode_accelerator.cc (right): http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:30: static const int kNumPictureBuffers = 5; Doco origin of this magic 5? http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:32: #define RETURN_ON_HR_FAILURE(result, log, ret_val) \ s/result/hr/ (it's funny that it works anyway; yay for consistently naming your variables "hr" :)) http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:32: #define RETURN_ON_HR_FAILURE(result, log, ret_val) \ I'm surprised this doesn't NotifyError. http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:34: if (FAILED(hr)) { \ You also have lots of occurrences of error-checking of bools (as opposed to HRESULTs) which seem like they could benefit from a macro. Again, see OVDA for the two variants (and how one reuses the other, including the formatting of the more-complex error code as part of the msg of the bool version; I think that will translate 1-1 to this class). http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:35: DVLOG(1) << log; \ Not worth emitting result itself? http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:91: HRESULT hr = E_FAIL; Why not declare & initialize to the real result you want (2 lines down) in one statement? (applies here and elsewhere; please do a sweep) http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:100: remove newline http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:148: bool is_available, const media::PictureBuffer& buffer) Does the ctor ever get called with an is_available=false? If not, can drop the param. http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:174: method_factory_.reset( Unused, which is unfortunate (see my comment about Unretained elsewhere). I suspect this is the right way to go. http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:183: bool DXVAVideoDecodeAccelerator::Initialize(Profile profile) { s/ profile// if unused http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:192: if (FAILED(hr)) { use macro? (here and elsewhere; please do a sweep) http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:197: if (!CreateD3DDevManager()) l.197-208 can be: if (!CreateD3DDevManager() || !InitDecoder() || !GetStreamsInfoAndBufferReqs() || !!SendMFTMessage(MFT_MESSAGE_NOTIFY_BEGIN_STREAMING, 0)) { return false; } Up to you. http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:212: base::Unretained(client_))); base::Unretained() should make you wonder about lifetimes. In particular I think almost all the usages of it in this file (around client_) are wrong and can cause crashes due to client_ going away between the posting of the task and its execution. Look at OVDA for one way to deal with this (take advantage of OVDA's refcountedness to post callbacks on itself instead of client_, which then call the client_ methods). http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:220: NOTREACHED() << "ConsumeVideoSample: invalid state"; NOTREACHED() should be reserved for detecting programming errors in chromium code. This branch can be triggered by requesting a reset and sending a decode (from the plugin) before the reset is done, for example, so doesn't qualify. At any rate, you should transition to error. http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:258: state_ = kEosDrain; I still don't understand this. What happens if the plugin sends multiple Decode()'s in in rapid succession? http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:270: // The Microsoft Media foundation decoder internally buffers upto 30 fps s/upto/up to/ s/30fps/30 frames/ http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:273: // Note: This may break clients which expect every input buffer to be Wouldn't it be better to not do this (retain the semantics of NotifyEndOfBitstreamBuffer in chromium/ppapi) and instead have such plugins simply simulate their own NotifyEndOfBitstreamBuffer immediately after every call to Decode()? http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:289: DCHECK(output_picture_buffers_.find(buffers[buffer_index].id()) == I think you missed my point. By calling insert() and checking .second you get to assert that the insert succeeded (i.e. the key wasn't already present), and avoid the double lookup (once for find and once for operator[]). http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:302: DVLOG(1) << "Invalid picture id"; NotifyError http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:312: VLOG(1) << "DXVAVideoDecodeAccelerator::Flush"; I think you missed my comment to s/VLOG/DVLOG/ everywhere in the CL. http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:320: DCHECK(state_ != kEosDrain); Redundant to the if above. http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:336: DoDecode(); Given DoDecode() is synchronous does this loop body ever actually execute? http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:343: state_ = kNormal;} missing newline before brace. http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:350: bool success = false; unused? http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:358: DCHECK(state_ != kResetting); redundant http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:362: VLOG(1) << "DXVAVideoDecodeAccelerator::Flush failed to send message"; s/Flush/Reset/ http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:383: &media::VideoDecodeAccelerator::Client::DismissPictureBuffer, This violates Destroy's API http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/ppapi/api/dev/ppb_vid... http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:397: Direct3DCreate9Ex(D3D_SDK_VERSION, d3d9.Receive()); Just to make sure you didn't miss them, apatrick made a bunch of comments around here. http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:571: // No more output from the decoder. Notify EOS and stop playback. Comment is at odds with the following NOTREACHED. I think the comment is wrong, but not sure. http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:574: // No more output from the decoder. Stop playback. copy/paste from the NEED_MORE_INPUT case below? I don't see why you're interpreting STREAM_CHANGE as a kStopped. http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:578: // No more output from the decoder. Stop playback. Add a DCHECK_EQ(state_, kEosDrain); i.e. that we were waiting for this to hit. http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:590: inputs_before_decode_); Can this ever *not* be 1, given the synchronous pairing of Decode/DoDecode? http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:692: glGetIntegerv(GL_TEXTURE_BINDING_2D, ¤t_texture); I'm surprised to see these (global-scope) gl* calls. Is there no context that needs to be made current before you can do these? http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:695: glTexImage2D(GL_TEXTURE_2D, 0, GL_BGRA_EXT, surface_desc.Width, How is this safe without a glFinish? http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:725: pending_output_samples_.erase(pending_output_samples_.begin()); pop_front() http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:727: if (pending_output_samples_.empty()) If you move this to the for-loop test you can drop the if/break here. http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:739: void DXVAVideoDecodeAccelerator::NotifyError( I think you actually want a StopOnError-equivalent that not only notifies the client but also transitions *this* class into an error state so no further attempts at work are made. http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:793: surface->ReleaseDC(hdc); combine into if below, or error-check otherwise? http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/dx... File content/common/gpu/media/dxva_video_decode_accelerator.h (right): http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.h:17: #include "build/build_config.h" What for? http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.h:59: // passed to the IMFTransform interface implemented by the H264 decoder. s/H264/h.264/ here and elsewhere in the CL. http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.h:100: // Processes pending output samples by copying them to available picture precede with newline http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.h:132: base::win::ScopedComPtr<IDirect3DSurface9> dest_surface; This field makes it ~PendingSampleInfo() non-trivial, so you should declare it here and define it (empty) in the .cc file. http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.h:135: typedef std::vector<PendingSampleInfo> PendingOutputSamples; Given the ways this is used a list<> makes more sense than a vector<> (since you want to support efficient pop_front). http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.h:137: // List of output samples for DXVAVideoDecodeAcceleratorrendering. s/rr/r r/ http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.h:144: DXVAPictureBuffer(); Can you drop this ctor? (I suspect you needed it only b/c of the use(s) of operator[], which I think isn't the right thing to use in your cases). http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.h:158: // Contains the id of the last input buffer received from the client. Can you drop this in favor of simply passing the buffer's id from Decode to DoDecode? http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.h:164: uint32 dev_manager_reset_token_; This needs a great big comment on it, if its only reason for existing is that the MF API can't be told to forget its argument. apatrick's comment started with "I think"; can you verify this in fact happens, since it's such a hack (or maybe you understand apatrick's tone better and know he meant "know" when he said "think" :)) http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/gp... File content/common/gpu/media/gpu_video_decode_accelerator.cc (right): http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/gp... content/common/gpu/media/gpu_video_decode_accelerator.cc:23: #if (defined(OS_CHROMEOS) && defined(ARCH_CPU_ARMEL)) || defined(OS_WIN) This can be simplified to: #if cros && armel #include omx #elif win #include dxva #endif http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/vi... File content/common/gpu/media/video_decode_accelerator_unittest.cc (left): http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/vi... content/common/gpu/media/video_decode_accelerator_unittest.cc:25: #include "testing/gtest/include/gtest/gtest.h" See the comment above this line for why it's included out-of-order. http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/vi... content/common/gpu/media/video_decode_accelerator_unittest.cc:323: done->Signal(); Since neither impl does anything after calling this, I think done should be signalled from the base impl (even if you didn't take my advice elsewhere to de-virtual'ize Initialize/UnInitialize). http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/vi... File content/common/gpu/media/video_decode_accelerator_unittest.cc (right): http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/vi... content/common/gpu/media/video_decode_accelerator_unittest.cc:40: #if (defined(OS_CHROMEOS) && defined(ARCH_CPU_ARMEL)) || defined(OS_WIN) If you negate this up here & #error out, then afterwards you can just check for one or the other. #if (!cros||!armel) && !win #error #endif #if win ... #else (can assume cros & arm) #endif http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/vi... content/common/gpu/media/video_decode_accelerator_unittest.cc:73: std::string test_video_data = "test-25fps.h264:320:240:250:258:50:175:1"; style guide prohibits global vars of class type, which is why this was a char* to begin with. http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/vi... content/common/gpu/media/video_decode_accelerator_unittest.cc:128: // Provides base functionality for managing EGL and GLES2 resources. Why move this below CreateShader above? It makes diff'ing harder than it needs to be. http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/vi... content/common/gpu/media/video_decode_accelerator_unittest.cc:132: // TODO(fischman): consider moving this into media/ if we can de-dup some of TODO can be dropped now that the media/ version of the copy/paste is deleted :) http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/vi... content/common/gpu/media/video_decode_accelerator_unittest.cc:145: // then all the usual work is done, except for the final swap of the EGL s/EGL// ? http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/vi... content/common/gpu/media/video_decode_accelerator_unittest.cc:148: virtual void Initialize( You've introduced a de-facto requirement that subclasses populate egl_display_ in their Initialize() before calling this base implementation. IWBN if instead you extract that part to mirror PlatformCreateWindow(). I think if you do that then this can stop being virtual. (taking it further, I think it would be best if this class only contained pure-virtuals and non-virtuals). http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/vi... content/common/gpu/media/video_decode_accelerator_unittest.cc:157: int top_left_y) PURE; Is PURE cross-platform? I've only ever seen =0 in chromium code. http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/vi... content/common/gpu/media/video_decode_accelerator_unittest.cc:170: EGLDisplay egl_display() { single-line http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/vi... content/common/gpu/media/video_decode_accelerator_unittest.cc:174: EGLContext egl_context() { single line http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/vi... content/common/gpu/media/video_decode_accelerator_unittest.cc:189: int num_windows_; Never used. http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/vi... content/common/gpu/media/video_decode_accelerator_unittest.cc:263: // media/tools/player_x11/gles_video_renderer.cc, with some simplification Drop the Note: since that file is now gone. http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/vi... content/common/gpu/media/video_decode_accelerator_unittest.cc:336: CHECK(eglMakeCurrent(egl_display_, egl_surfaces_[window_id], Why is this before the trampoline? http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/vi... content/common/gpu/media/video_decode_accelerator_unittest.cc:401: void Clear(); This shadows the base-class Clear (which isn't virtual). Is this really as clear as can be? http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/vi... content/common/gpu/media/video_decode_accelerator_unittest.cc:406: Win32RenderingHelper::Win32RenderingHelper() Single line. http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/vi... content/common/gpu/media/video_decode_accelerator_unittest.cc:426: RenderingHelperBase::UnInitialize(done); No need to undo the eglGetDisplay() from Initialize? http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/vi... content/common/gpu/media/video_decode_accelerator_unittest.cc:441: L"Static", Wrap a bit more aggressively? http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/vi... content/common/gpu/media/video_decode_accelerator_unittest.cc:466: // Initialize all structures to prepare to render to one or more windows of Commentary isn't necessary for OVERRIDEs. Just preface the list of overridden methods with a // RenderingHelperBase implementation. http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/vi... content/common/gpu/media/video_decode_accelerator_unittest.cc:690: scoped_refptr<DXVAVideoDecodeAccelerator> decoder_; Can this simply store a scoped_refptr<media::VideoDecodeAccelerator> and avoid the #if? http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/vi... content/common/gpu/media/video_decode_accelerator_unittest.cc:1020: CHECK(file_util::ReadFileToString(FilePath(UTF8ToWide(test_video_file)), This is win-specific. Instead use FILE_PATH_LITERAL to account for char/wchar in these APIs. http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/vi... content/common/gpu/media/video_decode_accelerator_unittest.cc:1027: Win32RenderingHelper rendering_helper; A crazy idea appears: if you named both renderinghelper derived classes RenderingHelper than a lot of the #if'ing later on can go away (as well as the need to Bind to base-class methods and rely on virtual dispatch inside of Bind, which makes me worry). Crazy or crazy like a fox? http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/vi... content/common/gpu/media/video_decode_accelerator_unittest.cc:1199: #if defined(OS_WIN) FILE_PATH_LITERAL should let you lose this. http://codereview.chromium.org/8510039/diff/47004/content/content_tests.gypi File content/content_tests.gypi (right): http://codereview.chromium.org/8510039/diff/47004/content/content_tests.gypi#... content/content_tests.gypi:407: 'target_name': 'omx_video_decode_accelerator_unittest', Do you have a tegra2-based crosbook to test that your changes don't make ovdatest stop working? http://codereview.chromium.org/8510039/diff/47004/content/content_tests.gypi#... content/content_tests.gypi:426: ['OS=="win"', { Why duplicate the above target? De-dup (with conditional deps, obvs.)?
http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/dx... File content/common/gpu/media/dxva_video_decode_accelerator.cc (right): http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:30: static const int kNumPictureBuffers = 5; On 2011/12/16 07:38:54, Ami Fischman wrote: > Doco origin of this magic 5? Done. http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:32: #define RETURN_ON_HR_FAILURE(result, log, ret_val) \ On 2011/12/16 07:38:54, Ami Fischman wrote: > s/result/hr/ > (it's funny that it works anyway; yay for consistently naming your variables > "hr" :)) Done. http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:34: if (FAILED(hr)) { \ On 2011/12/16 07:38:54, Ami Fischman wrote: > You also have lots of occurrences of error-checking of bools (as opposed to > HRESULTs) which seem like they could benefit from a macro. Again, see OVDA for > the two variants (and how one reuses the other, including the formatting of the > more-complex error code as part of the msg of the bool version; I think that > will translate 1-1 to this class). Added two variants of these macros. One which notifies the client of the error and the other one which just returns. We need two variants because some of the callers here are not member functions. http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:35: DVLOG(1) << log; \ On 2011/12/16 07:38:54, Ami Fischman wrote: > Not worth emitting result itself? The callers now pass in the formatted string if needed. This macro just calls into the newly added RETURN_ON_FAILURE macro which checks the bool condition. http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:91: HRESULT hr = E_FAIL; On 2011/12/16 07:38:54, Ami Fischman wrote: > Why not declare & initialize to the real result you want (2 lines down) in one > statement? > > (applies here and elsewhere; please do a sweep) Done. http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:100: On 2011/12/16 07:38:54, Ami Fischman wrote: > remove newline Done. http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:148: bool is_available, const media::PictureBuffer& buffer) On 2011/12/16 07:38:54, Ami Fischman wrote: > Does the ctor ever get called with an is_available=false? If not, can drop the > param. It does with your suggestion of using insert to check if the insert operation succeeded. http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:183: bool DXVAVideoDecodeAccelerator::Initialize(Profile profile) { On 2011/12/16 07:38:54, Ami Fischman wrote: > s/ profile// if unused Done. http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:192: if (FAILED(hr)) { On 2011/12/16 07:38:54, Ami Fischman wrote: > use macro? > (here and elsewhere; please do a sweep) Done. http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:197: if (!CreateD3DDevManager()) On 2011/12/16 07:38:54, Ami Fischman wrote: > l.197-208 can be: > if (!CreateD3DDevManager() || !InitDecoder() || !GetStreamsInfoAndBufferReqs() > || > !!SendMFTMessage(MFT_MESSAGE_NOTIFY_BEGIN_STREAMING, 0)) { > return false; > } > Up to you. Changed with the new macros. http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:220: NOTREACHED() << "ConsumeVideoSample: invalid state"; On 2011/12/16 07:38:54, Ami Fischman wrote: > NOTREACHED() should be reserved for detecting programming errors in chromium > code. > This branch can be triggered by requesting a reset and sending a decode (from > the plugin) before the reset is done, for example, so doesn't qualify. > > At any rate, you should transition to error. Done. http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:270: // The Microsoft Media foundation decoder internally buffers upto 30 fps On 2011/12/16 07:38:54, Ami Fischman wrote: > s/upto/up to/ > s/30fps/30 frames/ Done. http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:273: // Note: This may break clients which expect every input buffer to be On 2011/12/16 07:38:54, Ami Fischman wrote: > Wouldn't it be better to not do this (retain the semantics of > NotifyEndOfBitstreamBuffer in chromium/ppapi) and instead have such plugins > simply simulate their own NotifyEndOfBitstreamBuffer immediately after every > call to Decode()? As per our discussion leaving this as is for now. For context the plugins like the GLES plugin send over a set of 7 input packets and then send input only when they receive a notification that the input was processed. Currently we don't have a mechanism to force the decoder to emit output frames right away. Needs some more investigation. http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:302: DVLOG(1) << "Invalid picture id"; On 2011/12/16 07:38:54, Ami Fischman wrote: > NotifyError Done. http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:312: VLOG(1) << "DXVAVideoDecodeAccelerator::Flush"; On 2011/12/16 07:38:54, Ami Fischman wrote: > I think you missed my comment to s/VLOG/DVLOG/ everywhere in the CL. Done. http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:320: DCHECK(state_ != kEosDrain); On 2011/12/16 07:38:54, Ami Fischman wrote: > Redundant to the if above. Done. http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:336: DoDecode(); On 2011/12/16 07:38:54, Ami Fischman wrote: > Given DoDecode() is synchronous does this loop body ever actually execute? What we are doing is as per msdn documentation. I have seen one command drain call returning multiple key frames. http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:343: state_ = kNormal;} On 2011/12/16 07:38:54, Ami Fischman wrote: > missing newline before brace. Done. http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:350: bool success = false; On 2011/12/16 07:38:54, Ami Fischman wrote: > unused? Done. http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:358: DCHECK(state_ != kResetting); On 2011/12/16 07:38:54, Ami Fischman wrote: > redundant Done. http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:362: VLOG(1) << "DXVAVideoDecodeAccelerator::Flush failed to send message"; On 2011/12/16 07:38:54, Ami Fischman wrote: > s/Flush/Reset/ Done. http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:383: &media::VideoDecodeAccelerator::Client::DismissPictureBuffer, On 2011/12/16 07:38:54, Ami Fischman wrote: > This violates Destroy's API > http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/ppapi/api/dev/ppb_vid... Done. http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:397: Direct3DCreate9Ex(D3D_SDK_VERSION, d3d9.Receive()); On 2011/12/16 07:38:54, Ami Fischman wrote: > Just to make sure you didn't miss them, apatrick made a bunch of comments around > here. Yeah. We use the Ex versions of the APIs for that reason. http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:571: // No more output from the decoder. Notify EOS and stop playback. On 2011/12/16 07:38:54, Ami Fischman wrote: > Comment is at odds with the following NOTREACHED. I think the comment is wrong, > but not sure. Not sure what we can do here. It is probably correct to indicate stopped either way. http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:574: // No more output from the decoder. Stop playback. On 2011/12/16 07:38:54, Ami Fischman wrote: > copy/paste from the NEED_MORE_INPUT case below? I don't see why you're > interpreting STREAM_CHANGE as a kStopped. A stream change needs further process input calls to get back valid decoder output, which is why we need to set the state to stopped. I added a comment in the code indicating this. http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:578: // No more output from the decoder. Stop playback. On 2011/12/16 07:38:54, Ami Fischman wrote: > Add a DCHECK_EQ(state_, kEosDrain); i.e. that we were waiting for this to hit. Done. http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:590: inputs_before_decode_); On 2011/12/16 07:38:54, Ami Fischman wrote: > Can this ever *not* be 1, given the synchronous pairing of Decode/DoDecode? Covered with the decoder buffering behavior. http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:692: glGetIntegerv(GL_TEXTURE_BINDING_2D, ¤t_texture); On 2011/12/16 07:38:54, Ami Fischman wrote: > I'm surprised to see these (global-scope) gl* calls. Is there no context that > needs to be made current before you can do these? When i step into these calls i always get a context. Will check with Al Patrick. http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:692: glGetIntegerv(GL_TEXTURE_BINDING_2D, ¤t_texture); On 2011/12/16 07:38:54, Ami Fischman wrote: > I'm surprised to see these (global-scope) gl* calls. Is there no context that > needs to be made current before you can do these? Based on a discussion with Al Patrick an open gl context always exists when we are in the context of an IPC. I added a DCHECK for this. http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:695: glTexImage2D(GL_TEXTURE_2D, 0, GL_BGRA_EXT, surface_desc.Width, On 2011/12/16 07:38:54, Ami Fischman wrote: > How is this safe without a glFinish? Again based on a discussion with Al Patrick not needed as all these open gl calls execute on the same thread. Whenever we switch contexts it would implicitly flush. http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:725: pending_output_samples_.erase(pending_output_samples_.begin()); On 2011/12/16 07:38:54, Ami Fischman wrote: > pop_front() Done. http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:727: if (pending_output_samples_.empty()) On 2011/12/16 07:38:54, Ami Fischman wrote: > If you move this to the for-loop test you can drop the if/break here. Done. http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:739: void DXVAVideoDecodeAccelerator::NotifyError( On 2011/12/16 07:38:54, Ami Fischman wrote: > I think you actually want a StopOnError-equivalent that not only notifies the > client but also transitions *this* class into an error state so no further > attempts at work are made. Done. http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:793: surface->ReleaseDC(hdc); On 2011/12/16 07:38:54, Ami Fischman wrote: > combine into if below, or error-check otherwise? Done. http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/dx... File content/common/gpu/media/dxva_video_decode_accelerator.h (right): http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.h:17: #include "build/build_config.h" On 2011/12/16 07:38:54, Ami Fischman wrote: > What for? Removed. http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.h:59: // passed to the IMFTransform interface implemented by the H264 decoder. On 2011/12/16 07:38:54, Ami Fischman wrote: > s/H264/h.264/ here and elsewhere in the CL. Done. http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.h:100: // Processes pending output samples by copying them to available picture On 2011/12/16 07:38:54, Ami Fischman wrote: > precede with newline Done. http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.h:132: base::win::ScopedComPtr<IDirect3DSurface9> dest_surface; On 2011/12/16 07:38:54, Ami Fischman wrote: > This field makes it ~PendingSampleInfo() non-trivial, so you should declare it > here and define it (empty) in the .cc file. Done. http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.h:135: typedef std::vector<PendingSampleInfo> PendingOutputSamples; On 2011/12/16 07:38:54, Ami Fischman wrote: > Given the ways this is used a list<> makes more sense than a vector<> (since you > want to support efficient pop_front). Done. http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.h:137: // List of output samples for DXVAVideoDecodeAcceleratorrendering. On 2011/12/16 07:38:54, Ami Fischman wrote: > s/rr/r r/ Done. http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.h:144: DXVAPictureBuffer(); On 2011/12/16 07:38:54, Ami Fischman wrote: > Can you drop this ctor? > (I suspect you needed it only b/c of the use(s) of operator[], which I think > isn't the right thing to use in your cases). Done. http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.h:158: // Contains the id of the last input buffer received from the client. On 2011/12/16 07:38:54, Ami Fischman wrote: > Can you drop this in favor of simply passing the buffer's id from Decode to > DoDecode? DoDecode also gets called from Flush which drains the decoder. We need the last input buffer id to be sent over in the corresponding PictureReady message going over to the plugin. This is not very elegant and hopefully will be addressed with the buffering research. http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.h:164: uint32 dev_manager_reset_token_; On 2011/12/16 07:38:54, Ami Fischman wrote: > This needs a great big comment on it, if its only reason for existing is that > the MF API can't be told to forget its argument. > apatrick's comment started with "I think"; can you verify this in fact happens, > since it's such a hack (or maybe you understand apatrick's tone better and know > he meant "know" when he said "think" :)) Added a comment for now. Will look into this along with the buffering research needed into MF. http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/gp... File content/common/gpu/media/gpu_video_decode_accelerator.cc (right): http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/gp... content/common/gpu/media/gpu_video_decode_accelerator.cc:23: #if (defined(OS_CHROMEOS) && defined(ARCH_CPU_ARMEL)) || defined(OS_WIN) On 2011/12/16 07:38:54, Ami Fischman wrote: > This can be simplified to: > #if cros && armel > #include omx > #elif win > #include dxva > #endif Left this as is as we also need the "ui/gfx/gl/gl_context.h and ui/gfx/gl/gl_surface_egl.h for the decoders for omx and windows. http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/vi... File content/common/gpu/media/video_decode_accelerator_unittest.cc (left): http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/vi... content/common/gpu/media/video_decode_accelerator_unittest.cc:25: #include "testing/gtest/include/gtest/gtest.h" On 2011/12/16 07:38:54, Ami Fischman wrote: > See the comment above this line for why it's included out-of-order. Sorry about that. Restored. I did this to fix a lint error :( http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/vi... File content/common/gpu/media/video_decode_accelerator_unittest.cc (right): http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/vi... content/common/gpu/media/video_decode_accelerator_unittest.cc:40: #if (defined(OS_CHROMEOS) && defined(ARCH_CPU_ARMEL)) || defined(OS_WIN) On 2011/12/16 07:38:54, Ami Fischman wrote: > If you negate this up here & #error out, then afterwards you can just check for > one or the other. > > #if (!cros||!armel) && !win > #error > #endif > > #if win > ... > #else > (can assume cros & arm) > #endif Done. http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/vi... content/common/gpu/media/video_decode_accelerator_unittest.cc:73: std::string test_video_data = "test-25fps.h264:320:240:250:258:50:175:1"; On 2011/12/16 07:38:54, Ami Fischman wrote: > style guide prohibits global vars of class type, which is why this was a char* > to begin with. Restored. http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/vi... content/common/gpu/media/video_decode_accelerator_unittest.cc:128: // Provides base functionality for managing EGL and GLES2 resources. On 2011/12/16 07:38:54, Ami Fischman wrote: > Why move this below CreateShader above? It makes diff'ing harder than it needs > to be. Moved this back to just before Initialize. The CreateTexture method in this class needs to access the http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/vi... content/common/gpu/media/video_decode_accelerator_unittest.cc:132: // TODO(fischman): consider moving this into media/ if we can de-dup some of On 2011/12/16 07:38:54, Ami Fischman wrote: > TODO can be dropped now that the media/ version of the copy/paste is deleted :) Done. http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/vi... content/common/gpu/media/video_decode_accelerator_unittest.cc:145: // then all the usual work is done, except for the final swap of the EGL On 2011/12/16 07:38:54, Ami Fischman wrote: > s/EGL// ? I just copied this as is from the existing comment. http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/vi... content/common/gpu/media/video_decode_accelerator_unittest.cc:148: virtual void Initialize( On 2011/12/16 07:38:54, Ami Fischman wrote: > You've introduced a de-facto requirement that subclasses populate egl_display_ > in their Initialize() before calling this base implementation. IWBN if instead > you extract that part to mirror PlatformCreateWindow(). > I think if you do that then this can stop being virtual. > (taking it further, I think it would be best if this class only contained > pure-virtuals and non-virtuals). Done. http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/vi... content/common/gpu/media/video_decode_accelerator_unittest.cc:157: int top_left_y) PURE; On 2011/12/16 07:38:54, Ami Fischman wrote: > Is PURE cross-platform? I've only ever seen =0 in chromium code. Done. http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/vi... content/common/gpu/media/video_decode_accelerator_unittest.cc:170: EGLDisplay egl_display() { On 2011/12/16 07:38:54, Ami Fischman wrote: > single-line Done. http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/vi... content/common/gpu/media/video_decode_accelerator_unittest.cc:174: EGLContext egl_context() { On 2011/12/16 07:38:54, Ami Fischman wrote: > single line Done. http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/vi... content/common/gpu/media/video_decode_accelerator_unittest.cc:189: int num_windows_; On 2011/12/16 07:38:54, Ami Fischman wrote: > Never used. Removed http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/vi... content/common/gpu/media/video_decode_accelerator_unittest.cc:263: // media/tools/player_x11/gles_video_renderer.cc, with some simplification On 2011/12/16 07:38:54, Ami Fischman wrote: > Drop the Note: since that file is now gone. Done. http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/vi... content/common/gpu/media/video_decode_accelerator_unittest.cc:336: CHECK(eglMakeCurrent(egl_display_, egl_surfaces_[window_id], On 2011/12/16 07:38:54, Ami Fischman wrote: > Why is this before the trampoline? Stupid mistake :( http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/vi... content/common/gpu/media/video_decode_accelerator_unittest.cc:401: void Clear(); On 2011/12/16 07:38:54, Ami Fischman wrote: > This shadows the base-class Clear (which isn't virtual). > Is this really as clear as can be? Replaced this with a virtual override combo PlatformInitialize/PlatformUnInitialize http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/vi... content/common/gpu/media/video_decode_accelerator_unittest.cc:406: Win32RenderingHelper::Win32RenderingHelper() On 2011/12/16 07:38:54, Ami Fischman wrote: > Single line. Done. http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/vi... content/common/gpu/media/video_decode_accelerator_unittest.cc:426: RenderingHelperBase::UnInitialize(done); On 2011/12/16 07:38:54, Ami Fischman wrote: > No need to undo the eglGetDisplay() from Initialize? That happens in the base class Clear function. We set it back to EGL_NO_DISPLAY there. http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/vi... content/common/gpu/media/video_decode_accelerator_unittest.cc:441: L"Static", On 2011/12/16 07:38:54, Ami Fischman wrote: > Wrap a bit more aggressively? Done. http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/vi... content/common/gpu/media/video_decode_accelerator_unittest.cc:466: // Initialize all structures to prepare to render to one or more windows of On 2011/12/16 07:38:54, Ami Fischman wrote: > Commentary isn't necessary for OVERRIDEs. Just preface the list of overridden > methods with a > // RenderingHelperBase implementation. Done. http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/vi... content/common/gpu/media/video_decode_accelerator_unittest.cc:690: scoped_refptr<DXVAVideoDecodeAccelerator> decoder_; On 2011/12/16 07:38:54, Ami Fischman wrote: > Can this simply store a scoped_refptr<media::VideoDecodeAccelerator> and avoid > the #if? Done. http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/vi... content/common/gpu/media/video_decode_accelerator_unittest.cc:1020: CHECK(file_util::ReadFileToString(FilePath(UTF8ToWide(test_video_file)), On 2011/12/16 07:38:54, Ami Fischman wrote: > This is win-specific. > Instead use FILE_PATH_LITERAL to account for char/wchar in these APIs. Done. http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/vi... content/common/gpu/media/video_decode_accelerator_unittest.cc:1027: Win32RenderingHelper rendering_helper; On 2011/12/16 07:38:54, Ami Fischman wrote: > A crazy idea appears: if you named both renderinghelper derived classes > RenderingHelper than a lot of the #if'ing later on can go away (as well as the > need to Bind to base-class methods and rely on virtual dispatch inside of Bind, > which makes me worry). > Crazy or crazy like a fox? Done. http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/vi... content/common/gpu/media/video_decode_accelerator_unittest.cc:1199: #if defined(OS_WIN) On 2011/12/16 07:38:54, Ami Fischman wrote: > FILE_PATH_LITERAL should let you lose this. Done. http://codereview.chromium.org/8510039/diff/47004/content/content_tests.gypi File content/content_tests.gypi (right): http://codereview.chromium.org/8510039/diff/47004/content/content_tests.gypi#... content/content_tests.gypi:407: 'target_name': 'omx_video_decode_accelerator_unittest', On 2011/12/16 07:38:54, Ami Fischman wrote: > Do you have a tegra2-based crosbook to test that your changes don't make > ovdatest stop working? Unfortunately no. I will check around and see http://codereview.chromium.org/8510039/diff/47004/content/content_tests.gypi#... content/content_tests.gypi:426: ['OS=="win"', { On 2011/12/16 07:38:54, Ami Fischman wrote: > Why duplicate the above target? > De-dup (with conditional deps, obvs.)? Done.
http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/dx... File content/common/gpu/media/dxva_video_decode_accelerator.cc (right): http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:174: method_factory_.reset( On 2011/12/16 07:38:54, Ami Fischman wrote: > Unused, which is unfortunate (see my comment about Unretained elsewhere). I > suspect this is the right way to go. Removed http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:212: base::Unretained(client_))); On 2011/12/16 07:38:54, Ami Fischman wrote: > base::Unretained() should make you wonder about lifetimes. > In particular I think almost all the usages of it in this file (around client_) > are wrong and can cause crashes due to client_ going away between the posting of > the task and its execution. > Look at OVDA for one way to deal with this (take advantage of OVDA's > refcountedness to post callbacks on itself instead of client_, which then call > the client_ methods). Done. http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:258: state_ = kEosDrain; On 2011/12/16 07:38:54, Ami Fischman wrote: > I still don't understand this. > What happens if the plugin sends multiple Decode()'s in in rapid succession? Every decode call is associated with a ProcessInput/ProcessOutput sequence. If the ProcessOutput call returns more data needed then the state transitions to stopped. If it returns valid data we return that back to the plugin and continue. http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:289: DCHECK(output_picture_buffers_.find(buffers[buffer_index].id()) == On 2011/12/16 07:38:54, Ami Fischman wrote: > I think you missed my point. By calling insert() and checking .second you get > to assert that the insert succeeded (i.e. the key wasn't already present), and > avoid the double lookup (once for find and once for operator[]). Done.
http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/dx... File content/common/gpu/media/dxva_video_decode_accelerator.cc (right): http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:148: bool is_available, const media::PictureBuffer& buffer) On 2011/12/17 00:40:25, ananta wrote: > On 2011/12/16 07:38:54, Ami Fischman wrote: > > Does the ctor ever get called with an is_available=false? If not, can drop > the > > param. > > It does with your suggestion of using insert to check if the insert operation > succeeded. Where? All I'm seeing is "true", never "false". http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:273: // Note: This may break clients which expect every input buffer to be On 2011/12/17 00:40:25, ananta wrote: > On 2011/12/16 07:38:54, Ami Fischman wrote: > > Wouldn't it be better to not do this (retain the semantics of > > NotifyEndOfBitstreamBuffer in chromium/ppapi) and instead have such plugins > > simply simulate their own NotifyEndOfBitstreamBuffer immediately after every > > call to Decode()? > > As per our discussion leaving this as is for now. For context the plugins like > the GLES plugin send over a set of 7 input packets and then send input only when > they receive a notification that the input was processed. Currently we don't > have a mechanism to force the decoder to emit output frames right away. Needs > some more investigation. Please add a TODO for this and point to a crbug. http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/dx... File content/common/gpu/media/dxva_video_decode_accelerator.h (right): http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.h:158: // Contains the id of the last input buffer received from the client. On 2011/12/17 00:40:25, ananta wrote: > On 2011/12/16 07:38:54, Ami Fischman wrote: > > Can you drop this in favor of simply passing the buffer's id from Decode to > > DoDecode? > > DoDecode also gets called from Flush which drains the decoder. We need the last > input buffer id to be sent over in the corresponding PictureReady message going > over to the plugin. This is not very elegant and hopefully will be addressed > with the buffering research. Decode's synchronous implementation means Flush() will never have anything to flush, though, no? http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/vi... File content/common/gpu/media/video_decode_accelerator_unittest.cc (right): http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/vi... content/common/gpu/media/video_decode_accelerator_unittest.cc:128: // Provides base functionality for managing EGL and GLES2 resources. On 2011/12/17 00:40:25, ananta wrote: > On 2011/12/16 07:38:54, Ami Fischman wrote: > > Why move this below CreateShader above? It makes diff'ing harder than it > needs > > to be. > Moved this back to just before Initialize. Hahahahahahaha - now it's harder to diff between patchsets! Don't worry about it. I'll live :) > The CreateTexture method in this class needs to access the Not sure what you meant to say here. http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/vi... content/common/gpu/media/video_decode_accelerator_unittest.cc:145: // then all the usual work is done, except for the final swap of the EGL On 2011/12/17 00:40:25, ananta wrote: > On 2011/12/16 07:38:54, Ami Fischman wrote: > > s/EGL// ? > I just copied this as is from the existing comment. My point was that in the DXVA case the surfaces aren't EGL, but I guess I was wrong. http://codereview.chromium.org/8510039/diff/67040/content/common/gpu/media/dx... File content/common/gpu/media/dxva_video_decode_accelerator.cc (right): http://codereview.chromium.org/8510039/diff/67040/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:36: #define RETURN_ON_FAILURE(result, log, ret) \ A single failure in CreateEmptySample will result in these *4* messages being emitted: "Unable to create an empty sample" "Failed to create sample" "Failed to create empty buffer for input" "Failed to create input sample" Is that useful? I suspect the nugget of usefulness is only present in the deepest level of the callstack. I suggest that logging only happen when an error is first detected (either as a return code from an API from outside this file, or in the outer-most layer the error is detected in handling a public method invocation). http://codereview.chromium.org/8510039/diff/67040/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:45: RETURN_ON_FAILURE(SUCCEEDED(result), log, ret); If you replaced log in this call with: log << ", HRESULT: 0x" << std::hex << result, you'd get a nicely formatted version of the failure code for free (in that callers wouldn't have to do it). Ditto for RETURN_AND_NOTIFY_ON_HR_FAILURE. http://codereview.chromium.org/8510039/diff/67040/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:73: RETURN_ON_FAILURE(sample.get(), "Failed to create sample", NULL); Here and elsewhere, I don't think you need the .get() since ScopedComPtr is-a scoped_refptr, which defines operator*(). http://codereview.chromium.org/8510039/diff/67040/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:196: hr, "MFStartup failed. Error:" << std::hex << std::showbase, I think you forgot the final hr on this line (but more than that, I think you should take my suggestion above and avoid having to duplicate the hex/showbase/hr business at each callsite). http://codereview.chromium.org/8510039/diff/67040/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:241: RETURN_AND_NOTIFY_ON_HR_FAILURE(decoder_->ProcessInput(0, sample.get(), 0), Macros use text-substitution at compile time, so passing the first arg this way will double-evaluate if you take my suggestion above to emit the failure code as part of the error message. You have to use a temporary to store the return value instead. For the bool macro variants you don't have a bug (b/c result is mentioned exactly once) but just thought I'd mention it so you can decide whether you think this is worth saving the bool local variable. http://codereview.chromium.org/8510039/diff/67040/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:244: if (state_ != DXVAVideoDecodeAccelerator::kEosDrain) { You have a check at the top of this function that state_ is normal or stopped, so this if is always true. http://codereview.chromium.org/8510039/diff/67040/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:248: state_ = kEosDrain; IIUC DoDecode() always sets state_ to normal or stopped when it returns. So I'm not sure what purpose it serves to set this kEosDrain. Can you explain? http://codereview.chromium.org/8510039/diff/67040/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:290: "Invalid picture id: " << picture_buffer_id, PLATFORM_FAILURE,); s/PLATFORM_FAILURE/INVALID_ARGUMENT/ http://codereview.chromium.org/8510039/diff/67040/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:301: RETURN_AND_NOTIFY_ON_FAILURE((state_ == kNormal || state_ != kStopped), s/!=/==/ ??? http://codereview.chromium.org/8510039/diff/67040/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:314: while (state_ != kStopped) { I don't understand why Decode() is content to call DoDecode() exactly once, but Flush() must call it in a loop. Concerns: 1) Isn't the plugin sending in NALU-sized chunks? There shouldn't be more than one keyframe per NALU. 2) If you're worried about a single DoDecode() call not draining the input buffer, what's to prevent the decoder from being over-filled with inputs, if a long video is decoded via many Decode() calls with no Flush() until hours (in video time) later? Will the decoder hold onto all that memory? 3) If the backup described in #2 happens, will the GPU process stop being responsive while draining lots of video? These all stem from the fact that everything is synchronous in this CL. I suspect you actually need a dedicated thread/messageloop to make this work right. http://codereview.chromium.org/8510039/diff/67040/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:350: client_->DismissPictureBuffer(index->second.picture_buffer.id()); This still violates Destroy's API. You must not touch client_ after Destroy() is *called*. http://codereview.chromium.org/8510039/diff/67040/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:509: RETURN_AND_NOTIFY_ON_FAILURE((state_ == kNormal || state_ == kEosDrain), Always entered in kEosDrain ATM. http://codereview.chromium.org/8510039/diff/67040/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:528: // No more output from the decoder. Notify EOS and stop playback. I don't understand this. Please have someone who knows MFT/DXVA review. http://codereview.chromium.org/8510039/diff/67040/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:537: DCHECK(state_ == kEosDrain || state_ == kNormal); redundant to top-of-method check. http://codereview.chromium.org/8510039/diff/67040/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:666: !pending_output_samples_.empty(); de-indent http://codereview.chromium.org/8510039/diff/67040/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:786: client_->ProvidePictureBuffers(kNumPictureBuffers, style-guide requires multi-line then clauses to have braces in if statements. http://codereview.chromium.org/8510039/diff/67040/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:792: // This task could execute after the decoder has been torn down. I don't follow this comment. Why should state_==kStopped prevent delivery? http://codereview.chromium.org/8510039/diff/67040/content/common/gpu/media/dx... File content/common/gpu/media/dxva_video_decode_accelerator.h (right): http://codereview.chromium.org/8510039/diff/67040/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.h:189: // changes. This needs to be verified. s/This/TODO(ananta): This/ http://codereview.chromium.org/8510039/diff/67040/content/common/gpu/media/gp... File content/common/gpu/media/gpu_video_decode_accelerator.cc (right): http://codereview.chromium.org/8510039/diff/67040/content/common/gpu/media/gp... content/common/gpu/media/gpu_video_decode_accelerator.cc:30: #include "ui/gfx/gl/gl_context.h" Now I see why you did that; it would be clearer IMO to put these above the subordinate #if's to emph that these are common code. http://codereview.chromium.org/8510039/diff/67040/content/common/gpu/media/vi... File content/common/gpu/media/video_decode_accelerator_unittest.cc (right): http://codereview.chromium.org/8510039/diff/67040/content/common/gpu/media/vi... content/common/gpu/media/video_decode_accelerator_unittest.cc:385: #if defined(OS_WIN) Is the sum-total of the difference between the two declarations of RenderingHelper now just the private data members? If so then I wonder if it makes more sense to simply have a single class (instead of also having the Base class) with a single declaration, inside of which there's a single #if for the private members, and then having the impl of methods that are virtual now be in another #if (outside the class). WDYT? Seems like you could reduce duplication by just #if/else/endif'ing those 1 & 3 lines, resp. http://codereview.chromium.org/8510039/diff/67040/content/common/gpu/media/vi... content/common/gpu/media/video_decode_accelerator_unittest.cc:441: explicit RenderingHelper(); Drop explicit. http://codereview.chromium.org/8510039/diff/67040/content/common/gpu/media/vi... content/common/gpu/media/video_decode_accelerator_unittest.cc:450: // Zero-out internal state. indent http://codereview.chromium.org/8510039/diff/67040/content/common/gpu/media/vi... content/common/gpu/media/video_decode_accelerator_unittest.cc:459: // Zero-out internal state. Helper for ctor & UnInitialize(). Drop comment? http://codereview.chromium.org/8510039/diff/67040/content/common/gpu/media/vi... content/common/gpu/media/video_decode_accelerator_unittest.cc:463: XSetWindowAttributes window_attributes_; This doesn't need to be a class member (it can just live in PlatformCreateWindow) http://codereview.chromium.org/8510039/diff/67040/content/common/gpu/media/vi... content/common/gpu/media/video_decode_accelerator_unittest.cc:695: decoder = new DXVAVideoDecodeAccelerator(this, assign in previous line
http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/dx... File content/common/gpu/media/dxva_video_decode_accelerator.cc (right): http://codereview.chromium.org/8510039/diff/47004/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:273: // Note: This may break clients which expect every input buffer to be On 2011/12/19 22:53:44, Ami Fischman wrote: > On 2011/12/17 00:40:25, ananta wrote: > > On 2011/12/16 07:38:54, Ami Fischman wrote: > > > Wouldn't it be better to not do this (retain the semantics of > > > NotifyEndOfBitstreamBuffer in chromium/ppapi) and instead have such plugins > > > simply simulate their own NotifyEndOfBitstreamBuffer immediately after every > > > call to Decode()? > > > > As per our discussion leaving this as is for now. For context the plugins like > > the GLES plugin send over a set of 7 input packets and then send input only > when > > they receive a notification that the input was processed. Currently we don't > > have a mechanism to force the decoder to emit output frames right away. Needs > > some more investigation. > > Please add a TODO for this and point to a crbug. Done. http://codereview.chromium.org/8510039/diff/67040/content/common/gpu/media/dx... File content/common/gpu/media/dxva_video_decode_accelerator.cc (right): http://codereview.chromium.org/8510039/diff/67040/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:36: #define RETURN_ON_FAILURE(result, log, ret) \ On 2011/12/19 22:53:44, Ami Fischman wrote: > A single failure in CreateEmptySample will result in these *4* messages being > emitted: > "Unable to create an empty sample" > "Failed to create sample" > "Failed to create empty buffer for input" > "Failed to create input sample" > Is that useful? > I suspect the nugget of usefulness is only present in the deepest level of the > callstack. > > I suggest that logging only happen when an error is first detected (either as a > return code from an API from outside this file, or in the outer-most layer the > error is detected in handling a public method invocation). Replaced the logging with a CHECK condition instead as per our discussion. I am not sure if the API's where we are using this macro with actually fail. Would help catch these crashes on the field. http://codereview.chromium.org/8510039/diff/67040/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:45: RETURN_ON_FAILURE(SUCCEEDED(result), log, ret); On 2011/12/19 22:53:44, Ami Fischman wrote: > If you replaced log in this call with: > log << ", HRESULT: 0x" << std::hex << result, > you'd get a nicely formatted version of the failure code for free (in that > callers wouldn't have to do it). > Ditto for RETURN_AND_NOTIFY_ON_HR_FAILURE. Done. http://codereview.chromium.org/8510039/diff/67040/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:73: RETURN_ON_FAILURE(sample.get(), "Failed to create sample", NULL); On 2011/12/19 22:53:44, Ami Fischman wrote: > Here and elsewhere, I don't think you need the .get() since ScopedComPtr is-a > scoped_refptr, which defines operator*(). Done. http://codereview.chromium.org/8510039/diff/67040/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:196: hr, "MFStartup failed. Error:" << std::hex << std::showbase, On 2011/12/19 22:53:44, Ami Fischman wrote: > I think you forgot the final hr on this line (but more than that, I think you > should take my suggestion above and avoid having to duplicate the > hex/showbase/hr business at each callsite). Done. http://codereview.chromium.org/8510039/diff/67040/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:241: RETURN_AND_NOTIFY_ON_HR_FAILURE(decoder_->ProcessInput(0, sample.get(), 0), On 2011/12/19 22:53:44, Ami Fischman wrote: > Macros use text-substitution at compile time, so passing the first arg this way > will double-evaluate if you take my suggestion above to emit the failure code as > part of the error message. > You have to use a temporary to store the return value instead. > > For the bool macro variants you don't have a bug (b/c result is mentioned > exactly once) but just thought I'd mention it so you can decide whether you > think this is worth saving the bool local variable. Done. http://codereview.chromium.org/8510039/diff/67040/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:244: if (state_ != DXVAVideoDecodeAccelerator::kEosDrain) { On 2011/12/19 22:53:44, Ami Fischman wrote: > You have a check at the top of this function that state_ is normal or stopped, > so this if is always true. Done. http://codereview.chromium.org/8510039/diff/67040/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:248: state_ = kEosDrain; On 2011/12/19 22:53:44, Ami Fischman wrote: > IIUC DoDecode() always sets state_ to normal or stopped when it returns. So I'm > not sure what purpose it serves to set this kEosDrain. Can you explain? DoDecode also gets called from Flush which sets the state to kEosDrain to indicate that we are draining the decoder. DoDecode can be called currently in the kEosDrain state or kNormal. The latter state occurs when DoDecode gets back a decoded output frame. If the h.264 decoder returns more data needed the state of the decoder transitions to stopped. When we receive more input to be decoded we switch the state to kEosDrain to indicate that we have more input to be processed. http://codereview.chromium.org/8510039/diff/67040/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:290: "Invalid picture id: " << picture_buffer_id, PLATFORM_FAILURE,); On 2011/12/19 22:53:44, Ami Fischman wrote: > s/PLATFORM_FAILURE/INVALID_ARGUMENT/ Done. http://codereview.chromium.org/8510039/diff/67040/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:301: RETURN_AND_NOTIFY_ON_FAILURE((state_ == kNormal || state_ != kStopped), On 2011/12/19 22:53:44, Ami Fischman wrote: > s/!=/==/ ??? Thanks for catching this :(. Fixed. http://codereview.chromium.org/8510039/diff/67040/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:314: while (state_ != kStopped) { On 2011/12/19 22:53:44, Ami Fischman wrote: > I don't understand why Decode() is content to call DoDecode() exactly once, but > Flush() must call it in a loop. > Concerns: > 1) Isn't the plugin sending in NALU-sized chunks? There shouldn't be more than > one keyframe per NALU. > 2) If you're worried about a single DoDecode() call not draining the input > buffer, what's to prevent the decoder from being over-filled with inputs, if a > long video is decoded via many Decode() calls with no Flush() until hours (in > video time) later? Will the decoder hold onto all that memory? > 3) If the backup described in #2 happens, will the GPU process stop being > responsive while draining lots of video? > > These all stem from the fact that everything is synchronous in this CL. I > suspect you actually need a dedicated thread/messageloop to make this work > right. As per our IM discussion, the MFT decoder can buffer upto 30 frames worth of input before emitting output. This while loop in flush attempts to drain as many frames as we can from that set. I added a comment in the code. http://codereview.chromium.org/8510039/diff/67040/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:350: client_->DismissPictureBuffer(index->second.picture_buffer.id()); On 2011/12/19 22:53:44, Ami Fischman wrote: > This still violates Destroy's API. > You must not touch client_ after Destroy() is *called*. Done. http://codereview.chromium.org/8510039/diff/67040/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:509: RETURN_AND_NOTIFY_ON_FAILURE((state_ == kNormal || state_ == kEosDrain), On 2011/12/19 22:53:44, Ami Fischman wrote: > Always entered in kEosDrain ATM. Please note that the DoDecode function gets called in a loop in Flush which can cause the decoder to transition to kNormal for valid output. http://codereview.chromium.org/8510039/diff/67040/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:528: // No more output from the decoder. Notify EOS and stop playback. On 2011/12/19 22:53:44, Ami Fischman wrote: > I don't understand this. Please have someone who knows MFT/DXVA review. We don't have anyone who is very familiar with DXVA. In case it helps please look here http://msdn.microsoft.com/en-us/library/windows/desktop/ee663587(v=vs.85).aspx Look for Implementing format changes for the output type. http://codereview.chromium.org/8510039/diff/67040/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:537: DCHECK(state_ == kEosDrain || state_ == kNormal); On 2011/12/19 22:53:44, Ami Fischman wrote: > redundant to top-of-method check. Removed http://codereview.chromium.org/8510039/diff/67040/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:666: !pending_output_samples_.empty(); On 2011/12/19 22:53:44, Ami Fischman wrote: > de-indent Done. http://codereview.chromium.org/8510039/diff/67040/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:786: client_->ProvidePictureBuffers(kNumPictureBuffers, On 2011/12/19 22:53:44, Ami Fischman wrote: > style-guide requires multi-line then clauses to have braces in if statements. Done. http://codereview.chromium.org/8510039/diff/67040/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:792: // This task could execute after the decoder has been torn down. On 2011/12/19 22:53:44, Ami Fischman wrote: > I don't follow this comment. Why should state_==kStopped prevent delivery? Replaced with a check for kUninitialized. http://codereview.chromium.org/8510039/diff/67040/content/common/gpu/media/dx... File content/common/gpu/media/dxva_video_decode_accelerator.h (right): http://codereview.chromium.org/8510039/diff/67040/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.h:189: // changes. This needs to be verified. On 2011/12/19 22:53:44, Ami Fischman wrote: > s/This/TODO(ananta): This/ Done. http://codereview.chromium.org/8510039/diff/67040/content/common/gpu/media/gp... File content/common/gpu/media/gpu_video_decode_accelerator.cc (right): http://codereview.chromium.org/8510039/diff/67040/content/common/gpu/media/gp... content/common/gpu/media/gpu_video_decode_accelerator.cc:30: #include "ui/gfx/gl/gl_context.h" On 2011/12/19 22:53:44, Ami Fischman wrote: > Now I see why you did that; it would be clearer IMO to put these above the > subordinate #if's to emph that these are common code. Done. This would break the sorted include order though leading to more lint errors. http://codereview.chromium.org/8510039/diff/67040/content/common/gpu/media/vi... File content/common/gpu/media/video_decode_accelerator_unittest.cc (right): http://codereview.chromium.org/8510039/diff/67040/content/common/gpu/media/vi... content/common/gpu/media/video_decode_accelerator_unittest.cc:385: #if defined(OS_WIN) On 2011/12/19 22:53:44, Ami Fischman wrote: > Is the sum-total of the difference between the two declarations of > RenderingHelper now just the private data members? If so then I wonder if it > makes more sense to simply have a single class (instead of also having the Base > class) with a single declaration, inside of which there's a single #if for the > private members, and then having the impl of methods that are virtual now be in > another #if (outside the class). WDYT? > > Seems like you could reduce duplication by just #if/else/endif'ing those 1 & 3 > lines, resp. Done. http://codereview.chromium.org/8510039/diff/67040/content/common/gpu/media/vi... content/common/gpu/media/video_decode_accelerator_unittest.cc:441: explicit RenderingHelper(); On 2011/12/19 22:53:44, Ami Fischman wrote: > Drop explicit. This class has not been deleted. Platform specific functions and variables are in their ifdefs as per your suggestion. http://codereview.chromium.org/8510039/diff/67040/content/common/gpu/media/vi... content/common/gpu/media/video_decode_accelerator_unittest.cc:450: // Zero-out internal state. On 2011/12/19 22:53:44, Ami Fischman wrote: > indent Class is now gone. http://codereview.chromium.org/8510039/diff/67040/content/common/gpu/media/vi... content/common/gpu/media/video_decode_accelerator_unittest.cc:463: XSetWindowAttributes window_attributes_; On 2011/12/19 22:53:44, Ami Fischman wrote: > This doesn't need to be a class member (it can just live in > PlatformCreateWindow) Done. http://codereview.chromium.org/8510039/diff/67040/content/common/gpu/media/vi... content/common/gpu/media/video_decode_accelerator_unittest.cc:695: decoder = new DXVAVideoDecodeAccelerator(this, On 2011/12/19 22:53:44, Ami Fischman wrote: > assign in previous line Done.
http://codereview.chromium.org/8510039/diff/67040/content/common/gpu/media/gp... File content/common/gpu/media/gpu_video_decode_accelerator.cc (right): http://codereview.chromium.org/8510039/diff/67040/content/common/gpu/media/gp... content/common/gpu/media/gpu_video_decode_accelerator.cc:30: #include "ui/gfx/gl/gl_context.h" On 2011/12/20 02:27:22, ananta wrote: > On 2011/12/19 22:53:44, Ami Fischman wrote: > > Now I see why you did that; it would be clearer IMO to put these above the > > subordinate #if's to emph that these are common code. > > Done. This would break the sorted include order though leading to more lint > errors. I don't follow. Are you saying there are lint errors now? can you paste them? ISTM #includes were already out-of-order before (b/c of the texture_manager.h line outside the #if), so not sure how my suggestion made things worse. http://codereview.chromium.org/8510039/diff/72007/content/common/gpu/media/dx... File content/common/gpu/media/dxva_video_decode_accelerator.cc (right): http://codereview.chromium.org/8510039/diff/72007/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:40: return ret; \ return after CHECK doesn't make sense (meaning you can drop the ret param from the macro, as well). Also, s/RETURN/CHECK/ here and below. http://codereview.chromium.org/8510039/diff/72007/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:75: RETURN_ON_FAILURE(sample, NULL); Once you make the lowest-level methods CHECK on failure, you can assume they don't fail higher up, so for example this R_O_F is unnecessary (as are lots of others below). http://codereview.chromium.org/8510039/diff/72007/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:525: // No more output from the decoder. Notify EOS and stop playback. Your MSDN link makes me think this comment is bogus. If the stream changes then you need to do the type dance again, but there can certainly be more output from the decoder. Call DoDecode() recursively? http://codereview.chromium.org/8510039/diff/72007/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:527: NOTREACHED() << "Failed to set decoder output media type"; Given your move to CHECKs above SDOMT can't return false, so this can go away. (more generally, any function that calls CHECK_ON_FAILURE can be assumed to return successfully so doesn't need a bool return type) http://codereview.chromium.org/8510039/diff/72007/content/common/gpu/media/dx... File content/common/gpu/media/dxva_video_decode_accelerator.h (right): http://codereview.chromium.org/8510039/diff/72007/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.h:189: // changes. TODO(ananta) I think you dropped the "This needs to be verified" :)
http://codereview.chromium.org/8510039/diff/67040/content/common/gpu/media/gp... File content/common/gpu/media/gpu_video_decode_accelerator.cc (right): http://codereview.chromium.org/8510039/diff/67040/content/common/gpu/media/gp... content/common/gpu/media/gpu_video_decode_accelerator.cc:30: #include "ui/gfx/gl/gl_context.h" On 2011/12/20 21:00:24, Ami Fischman wrote: > On 2011/12/20 02:27:22, ananta wrote: > > On 2011/12/19 22:53:44, Ami Fischman wrote: > > > Now I see why you did that; it would be clearer IMO to put these above the > > > subordinate #if's to emph that these are common code. > > > > Done. This would break the sorted include order though leading to more lint > > errors. > > I don't follow. Are you saying there are lint errors now? can you paste them? > ISTM #includes were already out-of-order before (b/c of the texture_manager.h > line outside the #if), so not sure how my suggestion made things worse. Lint errors below:- +#if (defined(OS_CHROMEOS) && defined(ARCH_CPU_ARMEL)) || defined(OS_WIN) #include "ui/gfx/gl/gl_context.h" #include "ui/gfx/gl/gl_surface_egl.h" +#if defined(OS_WIN) +#include "content/common/gpu/media/dxva_video_decode_accelerator.h" Include "content/common/gpu/media/dxva_video_decode_accelerator.h" not in alphabetical order +#else // OS_WIN +#include "content/common/gpu/media/omx_video_decode_accelerator.h" Include "content/common/gpu/media/omx_video_decode_accelerator.h" not in alphabetical order +#endif // OS_WIN #endif In the earlier patch i was only seeing one error for the dxva include stuff. http://codereview.chromium.org/8510039/diff/72007/content/common/gpu/media/dx... File content/common/gpu/media/dxva_video_decode_accelerator.cc (right): http://codereview.chromium.org/8510039/diff/72007/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:40: return ret; \ On 2011/12/20 21:00:24, Ami Fischman wrote: > return after CHECK doesn't make sense (meaning you can drop the ret param from > the macro, as well). Also, s/RETURN/CHECK/ here and below. Done. http://codereview.chromium.org/8510039/diff/72007/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:40: return ret; \ On 2011/12/20 21:00:24, Ami Fischman wrote: > return after CHECK doesn't make sense (meaning you can drop the ret param from > the macro, as well). Also, s/RETURN/CHECK/ here and below. Replaced the CHECK macro with a DLOG(ERROR) as per our discussion. http://codereview.chromium.org/8510039/diff/72007/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:75: RETURN_ON_FAILURE(sample, NULL); On 2011/12/20 21:00:24, Ami Fischman wrote: > Once you make the lowest-level methods CHECK on failure, you can assume they > don't fail higher up, so for example this R_O_F is unnecessary (as are lots of > others below). With the CHECK macro being replaced with a DLOG, we need to handle this failure here. http://codereview.chromium.org/8510039/diff/72007/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:525: // No more output from the decoder. Notify EOS and stop playback. On 2011/12/20 21:00:24, Ami Fischman wrote: > Your MSDN link makes me think this comment is bogus. > If the stream changes then you need to do the type dance again, but there can > certainly be more output from the decoder. Call DoDecode() recursively? Done. http://codereview.chromium.org/8510039/diff/72007/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:527: NOTREACHED() << "Failed to set decoder output media type"; On 2011/12/20 21:00:24, Ami Fischman wrote: > Given your move to CHECKs above SDOMT can't return false, so this can go away. > (more generally, any function that calls CHECK_ON_FAILURE can be assumed to > return successfully so doesn't need a bool return type) Ditto as per the CHECK macro comment above. We need to handle this failure here. http://codereview.chromium.org/8510039/diff/72007/content/common/gpu/media/dx... File content/common/gpu/media/dxva_video_decode_accelerator.h (right): http://codereview.chromium.org/8510039/diff/72007/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.h:189: // changes. TODO(ananta) On 2011/12/20 21:00:24, Ami Fischman wrote: > I think you dropped the "This needs to be verified" :) Done. :(
LGTM modulo nits & not knowing how the MF APIs work. I'm patching the CL in to my cros/arm chroot and am verifying it doesn't break the OMX case. http://codereview.chromium.org/8510039/diff/52023/content/common/gpu/media/dx... File content/common/gpu/media/dxva_video_decode_accelerator.cc (right): http://codereview.chromium.org/8510039/diff/52023/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:39: DLOG(ERROR) << "Failed ***"; \ Why not also emit the log? http://codereview.chromium.org/8510039/diff/52023/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:397: CheckDecoderDxvaSupport(); RETURN_ON_FAILURE ? http://codereview.chromium.org/8510039/diff/52023/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:525: // No more output from the decoder. Notify EOS and stop playback. This comment is still wrong. http://codereview.chromium.org/8510039/diff/52023/content/common/gpu/media/dx... File content/common/gpu/media/dxva_video_decode_accelerator.h (right): http://codereview.chromium.org/8510039/diff/52023/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.h:63: // Validates if the h.264 decoder supports hardware video acceleration. s/if/whether/
With the changes below, video_decode_accelerator_unittest & ppapi_example_gles2 both work correctly on cros/arm. http://codereview.chromium.org/8510039/diff/52023/content/common/gpu/media/vi... File content/common/gpu/media/video_decode_accelerator_unittest.cc (right): http://codereview.chromium.org/8510039/diff/52023/content/common/gpu/media/vi... content/common/gpu/media/video_decode_accelerator_unittest.cc:451: (CWBackPixel | CWOverrideRedirect), window_attributes); s/window_attributes/&window_attributes/ http://codereview.chromium.org/8510039/diff/52023/content/common/gpu/media/vi... content/common/gpu/media/video_decode_accelerator_unittest.cc:929: rendering_thread.StartWithOptions(options); Per IM convo cros/arm needs this to be TYPE_DEFAULT, not TYPE_UI. http://codereview.chromium.org/8510039/diff/52023/content/content_tests.gypi File content/content_tests.gypi (right): http://codereview.chromium.org/8510039/diff/52023/content/content_tests.gypi#... content/content_tests.gypi:412: '../base/allocator/allocator.gyp:allocator', This breaks compilation on cros/arm.
http://codereview.chromium.org/8510039/diff/52023/content/common/gpu/media/dx... File content/common/gpu/media/dxva_video_decode_accelerator.cc (right): http://codereview.chromium.org/8510039/diff/52023/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:39: DLOG(ERROR) << "Failed ***"; \ On 2011/12/21 00:39:15, Ami Fischman wrote: > Why not also emit the log? Done. http://codereview.chromium.org/8510039/diff/52023/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:397: CheckDecoderDxvaSupport(); On 2011/12/21 00:39:15, Ami Fischman wrote: > RETURN_ON_FAILURE ? Done. http://codereview.chromium.org/8510039/diff/52023/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:525: // No more output from the decoder. Notify EOS and stop playback. On 2011/12/21 00:39:15, Ami Fischman wrote: > This comment is still wrong. Done. http://codereview.chromium.org/8510039/diff/52023/content/common/gpu/media/dx... File content/common/gpu/media/dxva_video_decode_accelerator.h (right): http://codereview.chromium.org/8510039/diff/52023/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.h:63: // Validates if the h.264 decoder supports hardware video acceleration. On 2011/12/21 00:39:15, Ami Fischman wrote: > s/if/whether/ Done. http://codereview.chromium.org/8510039/diff/52023/content/common/gpu/media/vi... File content/common/gpu/media/video_decode_accelerator_unittest.cc (right): http://codereview.chromium.org/8510039/diff/52023/content/common/gpu/media/vi... content/common/gpu/media/video_decode_accelerator_unittest.cc:451: (CWBackPixel | CWOverrideRedirect), window_attributes); On 2011/12/21 01:41:16, Ami Fischman wrote: > s/window_attributes/&window_attributes/ Done. http://codereview.chromium.org/8510039/diff/52023/content/common/gpu/media/vi... content/common/gpu/media/video_decode_accelerator_unittest.cc:929: rendering_thread.StartWithOptions(options); On 2011/12/21 01:41:16, Ami Fischman wrote: > Per IM convo cros/arm needs this to be TYPE_DEFAULT, not TYPE_UI. Done. http://codereview.chromium.org/8510039/diff/52023/content/content_tests.gypi File content/content_tests.gypi (right): http://codereview.chromium.org/8510039/diff/52023/content/content_tests.gypi#... content/content_tests.gypi:412: '../base/allocator/allocator.gyp:allocator', On 2011/12/21 01:41:16, Ami Fischman wrote: > This breaks compilation on cros/arm. Made this conditional for windows.
The cros_tegra2 trybot failure is caused by the fact that the cros chrome ebuild file specifies to build the omx_test binary explicitly (http://codesearch.google.com/codesearch#search/&q=omx_video_decode_accelerato...). I guess you land your change and then change those ebuilds to point to the new binary. (best to upload/get approval on that CL first, since I don't really know the cros side at all) http://codereview.chromium.org/8510039/diff/78002/content/common/gpu/media/dx... File content/common/gpu/media/dxva_video_decode_accelerator.cc (right): http://codereview.chromium.org/8510039/diff/78002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:192: LoadLibrary(decoding_dlls[i]); What about failure? Set some global var preventing DXVAVDA's from being constructed? Or will the failure to load here prevent initialization from succeeding later? http://codereview.chromium.org/8510039/diff/78002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:207: // Best effort to load required decoding dlls. Drop this since it's called from GpuMain? (if you don't drop this, you need to reason about thread-safety of the static initialized_dlls var in LoadDecodingDlls()) http://codereview.chromium.org/8510039/diff/78002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:575: // No more output from the decoder. Notify EOS and stop playback. I still don't understand this comment. Should it say // Decoder didn't let us set NV12 output format. I don't know why this can happen. Give up in disgust. ? http://codereview.chromium.org/8510039/diff/78002/content/gpu/gpu_main.cc File content/gpu/gpu_main.cc (right): http://codereview.chromium.org/8510039/diff/78002/content/gpu/gpu_main.cc#new... content/gpu/gpu_main.cc:89: DXVAVideoDecodeAccelerator::LoadDecodingDlls(); You'll need to get content/gpu/OWNERS' say-so on this.
http://codereview.chromium.org/8510039/diff/78002/content/common/gpu/media/dx... File content/common/gpu/media/dxva_video_decode_accelerator.cc (right): http://codereview.chromium.org/8510039/diff/78002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:192: LoadLibrary(decoding_dlls[i]); On 2011/12/21 06:13:55, Ami Fischman wrote: > What about failure? Set some global var preventing DXVAVDA's from being > constructed? Or will the failure to load here prevent initialization from > succeeding later? Done. Failure to load here will prevent initialization from succeeding. However it happens in the form of a GPU process crash while loading the decoder dlls in the sandbox. Added a static member variable to the DXVAVideoDecodeAccelerator class which defaults to false. We set it to true when we load all required dlls. http://codereview.chromium.org/8510039/diff/78002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:207: // Best effort to load required decoding dlls. On 2011/12/21 06:13:55, Ami Fischman wrote: > Drop this since it's called from GpuMain? > (if you don't drop this, you need to reason about thread-safety of the static > initialized_dlls var in LoadDecodingDlls()) Done. Added a call to the LoadDecodingDlls from the unit test. http://codereview.chromium.org/8510039/diff/78002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:575: // No more output from the decoder. Notify EOS and stop playback. On 2011/12/21 06:13:55, Ami Fischman wrote: > I still don't understand this comment. Should it say > // Decoder didn't let us set NV12 output format. I don't know why this can > happen. Give up in disgust. > ? Done. http://codereview.chromium.org/8510039/diff/78002/content/gpu/gpu_main.cc File content/gpu/gpu_main.cc (right): http://codereview.chromium.org/8510039/diff/78002/content/gpu/gpu_main.cc#new... content/gpu/gpu_main.cc:89: DXVAVideoDecodeAccelerator::LoadDecodingDlls(); On 2011/12/21 06:13:55, Ami Fischman wrote: > You'll need to get content/gpu/OWNERS' say-so on this. Done.
On 2011/12/21 07:04:54, ananta wrote: > http://codereview.chromium.org/8510039/diff/78002/content/common/gpu/media/dx... > File content/common/gpu/media/dxva_video_decode_accelerator.cc (right): > > http://codereview.chromium.org/8510039/diff/78002/content/common/gpu/media/dx... > content/common/gpu/media/dxva_video_decode_accelerator.cc:192: > LoadLibrary(decoding_dlls[i]); > On 2011/12/21 06:13:55, Ami Fischman wrote: > > What about failure? Set some global var preventing DXVAVDA's from being > > constructed? Or will the failure to load here prevent initialization from > > succeeding later? > > Done. Failure to load here will prevent initialization from succeeding. However > it happens in the form of a GPU process crash while loading the decoder dlls in > the sandbox. Added a static member variable to the DXVAVideoDecodeAccelerator > class which defaults to false. We set it to true when we load all required dlls. > > http://codereview.chromium.org/8510039/diff/78002/content/common/gpu/media/dx... > content/common/gpu/media/dxva_video_decode_accelerator.cc:207: // Best effort to > load required decoding dlls. > On 2011/12/21 06:13:55, Ami Fischman wrote: > > Drop this since it's called from GpuMain? > > (if you don't drop this, you need to reason about thread-safety of the static > > initialized_dlls var in LoadDecodingDlls()) > > Done. Added a call to the LoadDecodingDlls from the unit test. > > http://codereview.chromium.org/8510039/diff/78002/content/common/gpu/media/dx... > content/common/gpu/media/dxva_video_decode_accelerator.cc:575: // No more output > from the decoder. Notify EOS and stop playback. > On 2011/12/21 06:13:55, Ami Fischman wrote: > > I still don't understand this comment. Should it say > > // Decoder didn't let us set NV12 output format. I don't know why this can > > happen. Give up in disgust. > > ? > > Done. > > http://codereview.chromium.org/8510039/diff/78002/content/gpu/gpu_main.cc > File content/gpu/gpu_main.cc (right): > > http://codereview.chromium.org/8510039/diff/78002/content/gpu/gpu_main.cc#new... > content/gpu/gpu_main.cc:89: DXVAVideoDecodeAccelerator::LoadDecodingDlls(); > On 2011/12/21 06:13:55, Ami Fischman wrote: > > You'll need to get content/gpu/OWNERS' say-so on this. > > Done. For GPU owners approval I updated the CL description to reflect this. apatrick is already on this CL and has been requested to take a peek at the gpu_main changes.
http://codereview.chromium.org/8510039/diff/85002/content/common/gpu/media/dx... File content/common/gpu/media/dxva_video_decode_accelerator.cc (right): http://codereview.chromium.org/8510039/diff/85002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:180: if (loaded_decoder_dlls_) DCHECK(!loaded_decoder_dlls_) instead to enforce this is called exactly once at program startup? http://codereview.chromium.org/8510039/diff/85002/content/common/gpu/media/dx... File content/common/gpu/media/dxva_video_decode_accelerator.h (right): http://codereview.chromium.org/8510039/diff/85002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.h:202: // Set to true if we all required decoder dlls were successfully loaded. s/we // http://codereview.chromium.org/8510039/diff/85002/content/common/gpu/media/vi... File content/common/gpu/media/video_decode_accelerator_unittest.cc (right): http://codereview.chromium.org/8510039/diff/85002/content/common/gpu/media/vi... content/common/gpu/media/video_decode_accelerator_unittest.cc:641: DXVAVideoDecodeAccelerator::LoadDecodingDlls(); This should move into main() to mimic the production code.
http://codereview.chromium.org/8510039/diff/85002/content/common/gpu/media/dx... File content/common/gpu/media/dxva_video_decode_accelerator.cc (right): http://codereview.chromium.org/8510039/diff/85002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:180: if (loaded_decoder_dlls_) On 2011/12/21 18:04:56, Ami Fischman wrote: > DCHECK(!loaded_decoder_dlls_) instead to enforce this is called exactly once at > program startup? Done. http://codereview.chromium.org/8510039/diff/85002/content/common/gpu/media/dx... File content/common/gpu/media/dxva_video_decode_accelerator.h (right): http://codereview.chromium.org/8510039/diff/85002/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.h:202: // Set to true if we all required decoder dlls were successfully loaded. On 2011/12/21 18:04:56, Ami Fischman wrote: > s/we // Done. http://codereview.chromium.org/8510039/diff/85002/content/common/gpu/media/vi... File content/common/gpu/media/video_decode_accelerator_unittest.cc (right): http://codereview.chromium.org/8510039/diff/85002/content/common/gpu/media/vi... content/common/gpu/media/video_decode_accelerator_unittest.cc:641: DXVAVideoDecodeAccelerator::LoadDecodingDlls(); On 2011/12/21 18:04:56, Ami Fischman wrote: > This should move into main() to mimic the production code. Done.
http://codereview.chromium.org/8510039/diff/80019/content/common/gpu/media/dx... File content/common/gpu/media/dxva_video_decode_accelerator.cc (right): http://codereview.chromium.org/8510039/diff/80019/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.cc:181: DCHECK(!!loaded_decoder_dlls_); Why !! ??
LGTM
gpu owners LGTM.
still lgtm http://codereview.chromium.org/8510039/diff/78017/content/common/gpu/media/dx... File content/common/gpu/media/dxva_video_decode_accelerator.h (right): http://codereview.chromium.org/8510039/diff/78017/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.h:212: // Set to true if any necessary initialization needed before the GPU process s/any/all/ http://codereview.chromium.org/8510039/diff/78017/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.h:213: // is sandboxed are done. s/are/is/ http://codereview.chromium.org/8510039/diff/78017/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.h:214: // This includes the following:- s/-//
LGTM with 1 fix. http://codereview.chromium.org/8510039/diff/78017/content/content_common.gypi File content/content_common.gypi (right): http://codereview.chromium.org/8510039/diff/78017/content/content_common.gypi... content/content_common.gypi:352: 'libGLESv2.dll', Per offline discussion, this line can be removed.
http://codereview.chromium.org/8510039/diff/78017/content/common/gpu/media/dx... File content/common/gpu/media/dxva_video_decode_accelerator.h (right): http://codereview.chromium.org/8510039/diff/78017/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.h:212: // Set to true if any necessary initialization needed before the GPU process On 2011/12/21 22:07:08, Ami Fischman wrote: > s/any/all/ Done. http://codereview.chromium.org/8510039/diff/78017/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.h:213: // is sandboxed are done. On 2011/12/21 22:07:08, Ami Fischman wrote: > s/are/is/ Done. http://codereview.chromium.org/8510039/diff/78017/content/common/gpu/media/dx... content/common/gpu/media/dxva_video_decode_accelerator.h:214: // This includes the following:- On 2011/12/21 22:07:08, Ami Fischman wrote: > s/-// Done.
LGTM |
