Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10388108:
Implement media::VideoDecodeAccelerator on Mac (Closed)
|
Created:
8 years, 7 months ago by sail Modified:
8 years, 6 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, feature-media-reviews_chromium.org, jam, apatrick_chromium, darin-cc_chromium.org, viettrungluu, Pawel Osciak Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionImplement media::VideoDecodeAccelerator on Mac
This CL implements media::VideoDecodeAccelerator for Mac. This will be used to implement hardware video decoding for Pepper Flash.
Currently a couple things are still missing:
- the new VideoDecodeAcceleratorMac class needs to be hooked up to GpuVideoDecodeAccelerator
- the PPB_VideoDecoder_Dev::Create() API needs to be extended so clients can specify the extra AVC data.
BUG=127414
TEST=Ran on Mac and Windows. Sending ARM try bots now.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=140153
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=140187
Patch Set 1 #Patch Set 2 : a #Patch Set 3 : b #Patch Set 4 : a #
Total comments: 49
Patch Set 5 : address review comments #
Total comments: 96
Patch Set 6 : patch #Patch Set 7 : fix typo #Patch Set 8 : remove #if 0 #
Total comments: 45
Patch Set 9 : address review comments #Patch Set 10 : remove #if 0 #Patch Set 11 : a #Patch Set 12 : a #
Total comments: 2
Patch Set 13 : address review comments #
Total comments: 6
Patch Set 14 : address review comments #Patch Set 15 : address review comments #
Total comments: 6
Patch Set 16 : address review comments #Patch Set 17 : fix build #Patch Set 18 : fix win build #Patch Set 19 : fix cros build #Messages
Total messages: 41 (0 generated)
fischman: video_decode_accelerator_unittest.cc and rendering_helper* rsesek, thakis: everything else
https://chromiumcodereview.appspot.com/10388108/diff/6/base/mac/scoped_refptr... File base/mac/scoped_refptr_cftype.h (right): https://chromiumcodereview.appspot.com/10388108/diff/6/base/mac/scoped_refptr... base/mac/scoped_refptr_cftype.h:21: class scoped_refptr_cftype { Needs a test. https://chromiumcodereview.appspot.com/10388108/diff/6/base/mac/scoped_refptr... base/mac/scoped_refptr_cftype.h:21: class scoped_refptr_cftype { I wonder if this should be named ScopedRefptrCFType https://chromiumcodereview.appspot.com/10388108/diff/6/base/mac/scoped_refptr... base/mac/scoped_refptr_cftype.h:62: // Retain first so that self assignment should work nit: full-stop https://chromiumcodereview.appspot.com/10388108/diff/6/base/mac/scoped_refptr... base/mac/scoped_refptr_cftype.h:91: protected: Why not private? https://chromiumcodereview.appspot.com/10388108/diff/6/content/common/gpu/med... File content/common/gpu/media/rendering_helper.h (right): https://chromiumcodereview.appspot.com/10388108/diff/6/content/common/gpu/med... content/common/gpu/media/rendering_helper.h:58: virtual MessageLoop* GetMessageLoop() = 0; Proxy? https://chromiumcodereview.appspot.com/10388108/diff/6/content/common/gpu/med... content/common/gpu/media/rendering_helper.h:61: nit: two extra blank lines https://chromiumcodereview.appspot.com/10388108/diff/6/content/common/gpu/med... File content/common/gpu/media/rendering_helper_egl.cc (right): https://chromiumcodereview.appspot.com/10388108/diff/6/content/common/gpu/med... content/common/gpu/media/rendering_helper_egl.cc:203: ); This looks far-flung. https://chromiumcodereview.appspot.com/10388108/diff/6/content/common/gpu/med... File content/common/gpu/media/rendering_helper_mac.mm (right): https://chromiumcodereview.appspot.com/10388108/diff/6/content/common/gpu/med... content/common/gpu/media/rendering_helper_mac.mm:123: [NSApplication sharedApplication]; What if this is called before +[CrApplication sharedApplication]? https://chromiumcodereview.appspot.com/10388108/diff/6/content/common/gpu/med... content/common/gpu/media/rendering_helper_mac.mm:151: CHECK(num_windows == 1); CHECK_EQ https://chromiumcodereview.appspot.com/10388108/diff/6/content/common/gpu/med... content/common/gpu/media/rendering_helper_mac.mm:178: done->Signal(); What thread is going to be waiting on this?
This is exciting! I had no idea you (or anyone!) was working on this. https://chromiumcodereview.appspot.com/10388108/diff/6/base/base.gypi File base/base.gypi (right): https://chromiumcodereview.appspot.com/10388108/diff/6/base/base.gypi#newcode1 base/base.gypi:1: # Copyright (c) 2012 The Chromium Authors. All rights reserved. Are you aware of GpuVideoDecoder? It exposes *VDA's to html5 <video> tags. That's a less convoluted code path than ppapi, so as a next step instead of trying to make flash/pepper work you might enjoy enabling the new VDA for mac, first. It should just be extending http://code.google.com/searchframe#OAMlx_jo-ck/src/content/renderer/render_vi... in a pretty straightforward way. https://chromiumcodereview.appspot.com/10388108/diff/6/base/mac/scoped_refptr... File base/mac/scoped_refptr_cftype.h (right): https://chromiumcodereview.appspot.com/10388108/diff/6/base/mac/scoped_refptr... base/mac/scoped_refptr_cftype.h:21: class scoped_refptr_cftype { Could you avoid the need for this class (and tests) by extending scoped_ptr_malloc to take not only a "release" functor but also an "acquire" functor (default would just be assignment, this class' version would be assign & CFRetain)? If you keep this class, WDYT of adding Pass() to it (aka MOVE_ONLY_TYPE_FOR_CPP_03)? https://chromiumcodereview.appspot.com/10388108/diff/6/content/common/gpu/med... File content/common/gpu/media/rendering_helper.h (right): https://chromiumcodereview.appspot.com/10388108/diff/6/content/common/gpu/med... content/common/gpu/media/rendering_helper.h:17: // Creates and draws textures used by the video decoder. Doco thread unsafety? Or is the mac impl more flexible (in which case move the original doco from vda_test.cc to r_h_egl.cc) https://chromiumcodereview.appspot.com/10388108/diff/6/content/common/gpu/med... content/common/gpu/media/rendering_helper.h:58: virtual MessageLoop* GetMessageLoop() = 0; Unused? https://chromiumcodereview.appspot.com/10388108/diff/6/content/common/gpu/med... File content/common/gpu/media/rendering_helper_egl.cc (right): https://chromiumcodereview.appspot.com/10388108/diff/6/content/common/gpu/med... content/common/gpu/media/rendering_helper_egl.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. I assume this file is more or less a copy of the text removed from vda_test.cc. Let me know if I should look more closely at it. https://chromiumcodereview.appspot.com/10388108/diff/6/content/common/gpu/med... content/common/gpu/media/rendering_helper_egl.cc:25: namespace { media code uses static for free functions of file scope (instead of anonymous namespace). https://chromiumcodereview.appspot.com/10388108/diff/6/content/common/gpu/med... content/common/gpu/media/rendering_helper_egl.cc:73: void PlatformInitialize(); It's slightly confusing that in one sense this whole file is a "per-platform" thing (EGL vs. NSOpenGL vs. someday GLX etc) but within this file there is still "per-platform" switching (win vs. cros/arm). I don't know what that means, just thought I'd throw this out there. https://chromiumcodereview.appspot.com/10388108/diff/6/content/common/gpu/med... File content/common/gpu/media/video_decode_accelerator_mac.h (right): https://chromiumcodereview.appspot.com/10388108/diff/6/content/common/gpu/med... content/common/gpu/media/video_decode_accelerator_mac.h:26: class VideoDecodeAcceleratorMac : public media::VideoDecodeAccelerator { FWIW, the other VDA's follow the convention FooVDA, not VDAFoo. https://chromiumcodereview.appspot.com/10388108/diff/6/content/common/gpu/med... content/common/gpu/media/video_decode_accelerator_mac.h:35: // TODO(sail): Move this into Initialize. Initialize() is a member of the VDA interface, and correspondingly of the IPC message used to initialize VDA's. width/height aren't a part of that API. Does the underlying video decode API you're using not support mid-stream dimensions change? IOW, is it possible to start out with a fake size of 0,0 and have the decoder divine from the stream what the media's dimensions are? https://chromiumcodereview.appspot.com/10388108/diff/6/content/common/gpu/med... File content/common/gpu/media/video_decode_accelerator_mac.mm (right): https://chromiumcodereview.appspot.com/10388108/diff/6/content/common/gpu/med... content/common/gpu/media/video_decode_accelerator_mac.mm:2: // Use of this source code is governed by a BSD-style license that can be I don't know anything about Objective-C so I'm going to leave reviewing this file to others. Please let me know if you want me to look at it deeper/at all. https://chromiumcodereview.appspot.com/10388108/diff/6/content/common/gpu/med... File content/common/gpu/media/video_decode_accelerator_unittest.cc (right): https://chromiumcodereview.appspot.com/10388108/diff/6/content/common/gpu/med... content/common/gpu/media/video_decode_accelerator_unittest.cc:79: // AAVC data required to initialize the H.264 video decoder. I don't know what this is, but: 1) can it move to live near its only use? 2) can it move into vda_mac.mm or does it really need to be set from the outside? If the latter, is this going to need to be plumbed from ppapi through the various IPC layers? 3) where do the values come from / what do they mean? https://chromiumcodereview.appspot.com/10388108/diff/6/content/common/gpu/med... content/common/gpu/media/video_decode_accelerator_unittest.cc:548: DeleteDecoder(); Should this be posting instead of calling in-line to account for outstanding Decode tasks from above, now? https://chromiumcodereview.appspot.com/10388108/diff/6/content/common/gpu/med... content/common/gpu/media/video_decode_accelerator_unittest.cc:751: #if 0 Blort? https://chromiumcodereview.appspot.com/10388108/diff/6/content/common/gpu/med... content/common/gpu/media/video_decode_accelerator_unittest.cc:840: DXVAVideoDecodeAccelerator::PreSandboxInitialization(); why not move this into the OS_WIN branch of RenderingHelper::InitializePlatform() as well? https://chromiumcodereview.appspot.com/10388108/diff/6/content/content_tests.... File content/content_tests.gypi (right): https://chromiumcodereview.appspot.com/10388108/diff/6/content/content_tests.... content/content_tests.gypi:485: 'target_name': 'video_decode_accelerator_mac_unittest', Having a target name per platform was an organic mistake (made sense when omx_vda was the only VDA); s/_mac// here to stop propagating that mistake? https://chromiumcodereview.appspot.com/10388108/diff/6/content/content_tests.... content/content_tests.gypi:487: '../media/media.gyp:media', unnecessary
https://chromiumcodereview.appspot.com/10388108/diff/6/base/mac/scoped_refptr... File base/mac/scoped_refptr_cftype.h (right): https://chromiumcodereview.appspot.com/10388108/diff/6/base/mac/scoped_refptr... base/mac/scoped_refptr_cftype.h:3: // found in the LICENSE file. scoped_nsobject recently received an extension to give it an ownership policy; I'd rather see you do something similar to scoped_cftyperef than create a new class. Or possibly just do foo.reset(CFRetain(bar)).
https://chromiumcodereview.appspot.com/10388108/diff/6/base/base.gypi File base/base.gypi (right): https://chromiumcodereview.appspot.com/10388108/diff/6/base/base.gypi#newcode1 base/base.gypi:1: # Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2012/05/15 17:55:22, Ami Fischman wrote: > Are you aware of GpuVideoDecoder? It exposes *VDA's to html5 <video> tags. > That's a less convoluted code path than ppapi, so as a next step instead of > trying to make flash/pepper work you might enjoy enabling the new VDA for mac, > first. It should just be extending > http://code.google.com/searchframe#OAMlx_jo-ck/src/content/renderer/render_vi... > in a pretty straightforward way. Yea, thats a good point. I have most of the PPAPI code path wired up so I'm going to try to land that first. As soon as that's in I'll work on the HTML5 code path. https://chromiumcodereview.appspot.com/10388108/diff/6/base/mac/scoped_refptr... File base/mac/scoped_refptr_cftype.h (right): https://chromiumcodereview.appspot.com/10388108/diff/6/base/mac/scoped_refptr... base/mac/scoped_refptr_cftype.h:3: // found in the LICENSE file. On 2012/05/15 18:04:19, Avi wrote: > scoped_nsobject recently received an extension to give it an ownership policy; > I'd rather see you do something similar to scoped_cftyperef than create a new > class. Or possibly just do foo.reset(CFRetain(bar)). Done. Added OwnershipPolicy to ScopedCFTypeRef and removed this class. https://chromiumcodereview.appspot.com/10388108/diff/6/base/mac/scoped_refptr... base/mac/scoped_refptr_cftype.h:21: class scoped_refptr_cftype { On 2012/05/15 17:55:22, Ami Fischman wrote: > Could you avoid the need for this class (and tests) by extending > scoped_ptr_malloc to take not only a "release" functor but also an "acquire" > functor (default would just be assignment, this class' version would be assign & > CFRetain)? > > If you keep this class, WDYT of adding Pass() to it (aka > MOVE_ONLY_TYPE_FOR_CPP_03)? I followed Avi's suggestion and used ScopedCFTypeRef instead. https://chromiumcodereview.appspot.com/10388108/diff/6/base/mac/scoped_refptr... base/mac/scoped_refptr_cftype.h:21: class scoped_refptr_cftype { On 2012/05/15 16:04:49, rsesek wrote: > Needs a test. Removed this class. https://chromiumcodereview.appspot.com/10388108/diff/6/content/common/gpu/med... File content/common/gpu/media/rendering_helper.h (right): https://chromiumcodereview.appspot.com/10388108/diff/6/content/common/gpu/med... content/common/gpu/media/rendering_helper.h:17: // Creates and draws textures used by the video decoder. On 2012/05/15 17:55:22, Ami Fischman wrote: > Doco thread unsafety? Or is the mac impl more flexible (in which case move the > original doco from vda_test.cc to r_h_egl.cc) Done. https://chromiumcodereview.appspot.com/10388108/diff/6/content/common/gpu/med... content/common/gpu/media/rendering_helper.h:58: virtual MessageLoop* GetMessageLoop() = 0; On 2012/05/15 17:55:22, Ami Fischman wrote: > Unused? Done. Yea, it turns out that this wasn't needed. Removed. https://chromiumcodereview.appspot.com/10388108/diff/6/content/common/gpu/med... content/common/gpu/media/rendering_helper.h:61: On 2012/05/15 16:04:49, rsesek wrote: > nit: two extra blank lines Done. https://chromiumcodereview.appspot.com/10388108/diff/6/content/common/gpu/med... File content/common/gpu/media/rendering_helper_egl.cc (right): https://chromiumcodereview.appspot.com/10388108/diff/6/content/common/gpu/med... content/common/gpu/media/rendering_helper_egl.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2012/05/15 17:55:22, Ami Fischman wrote: > I assume this file is more or less a copy of the text removed from vda_test.cc. > Let me know if I should look more closely at it. Yea, this is mostly copy paste. https://chromiumcodereview.appspot.com/10388108/diff/6/content/common/gpu/med... content/common/gpu/media/rendering_helper_egl.cc:25: namespace { On 2012/05/15 17:55:22, Ami Fischman wrote: > media code uses static for free functions of file scope (instead of anonymous > namespace). Done. Changed rendering_helper_egl.cc, rendering_helper_mac.cc, and video_decode_accelerator_mac.mm to use static functions. https://chromiumcodereview.appspot.com/10388108/diff/6/content/common/gpu/med... content/common/gpu/media/rendering_helper_egl.cc:73: void PlatformInitialize(); On 2012/05/15 17:55:22, Ami Fischman wrote: > It's slightly confusing that in one sense this whole file is a "per-platform" > thing (EGL vs. NSOpenGL vs. someday GLX etc) but within this file there is still > "per-platform" switching (win vs. cros/arm). I don't know what that means, just > thought I'd throw this out there. Yea, I agree this sucks but I couldn't think of any solutions. Maybe once we have another platform we'll abe able to come up with a better organization. https://chromiumcodereview.appspot.com/10388108/diff/6/content/common/gpu/med... content/common/gpu/media/rendering_helper_egl.cc:203: ); On 2012/05/15 16:04:49, rsesek wrote: > This looks far-flung. Done. https://chromiumcodereview.appspot.com/10388108/diff/6/content/common/gpu/med... File content/common/gpu/media/rendering_helper_mac.mm (right): https://chromiumcodereview.appspot.com/10388108/diff/6/content/common/gpu/med... content/common/gpu/media/rendering_helper_mac.mm:123: [NSApplication sharedApplication]; On 2012/05/15 16:04:49, rsesek wrote: > What if this is called before +[CrApplication sharedApplication]? Hm... I don't understand how this works. Can you provide a link where this is declared? Since this code is just for the unittest do I still need it? https://chromiumcodereview.appspot.com/10388108/diff/6/content/common/gpu/med... content/common/gpu/media/rendering_helper_mac.mm:151: CHECK(num_windows == 1); On 2012/05/15 16:04:49, rsesek wrote: > CHECK_EQ Done. https://chromiumcodereview.appspot.com/10388108/diff/6/content/common/gpu/med... content/common/gpu/media/rendering_helper_mac.mm:178: done->Signal(); On 2012/05/15 16:04:49, rsesek wrote: > What thread is going to be waiting on this? The main thread will be waiting for this. See: http://code.google.com/searchframe#OAMlx_jo-ck/src/content/common/gpu/media/v... This function is run on the rendering thread. https://chromiumcodereview.appspot.com/10388108/diff/6/content/common/gpu/med... File content/common/gpu/media/video_decode_accelerator_mac.h (right): https://chromiumcodereview.appspot.com/10388108/diff/6/content/common/gpu/med... content/common/gpu/media/video_decode_accelerator_mac.h:26: class VideoDecodeAcceleratorMac : public media::VideoDecodeAccelerator { On 2012/05/15 17:55:22, Ami Fischman wrote: > FWIW, the other VDA's follow the convention FooVDA, not VDAFoo. Done. https://chromiumcodereview.appspot.com/10388108/diff/6/content/common/gpu/med... content/common/gpu/media/video_decode_accelerator_mac.h:35: // TODO(sail): Move this into Initialize. On 2012/05/15 17:55:22, Ami Fischman wrote: > Initialize() is a member of the VDA interface, and correspondingly of the IPC > message used to initialize VDA's. width/height aren't a part of that API. > > Does the underlying video decode API you're using not support mid-stream > dimensions change? IOW, is it possible to start out with a fake size of 0,0 and > have the decoder divine from the stream what the media's dimensions are? These are the dimensions stored in the extra_data() portion of the H.264 stream. See: http://code.google.com/searchframe#OAMlx_jo-ck/src/media/base/video_decoder_c... Unfortunately the Mac decoder will fail to initialize without this data. Passing 0,0 doesn't see to work either. My plan was to extend the IPC message with these 3 parameters and mark them as optional. https://chromiumcodereview.appspot.com/10388108/diff/6/content/common/gpu/med... File content/common/gpu/media/video_decode_accelerator_mac.mm (right): https://chromiumcodereview.appspot.com/10388108/diff/6/content/common/gpu/med... content/common/gpu/media/video_decode_accelerator_mac.mm:2: // Use of this source code is governed by a BSD-style license that can be On 2012/05/15 17:55:22, Ami Fischman wrote: > I don't know anything about Objective-C so I'm going to leave reviewing this > file to others. Please let me know if you want me to look at it deeper/at all. It would be great if you could look at the way I'm managing the PictureBuffer list and decoded frame list. https://chromiumcodereview.appspot.com/10388108/diff/6/content/common/gpu/med... File content/common/gpu/media/video_decode_accelerator_unittest.cc (right): https://chromiumcodereview.appspot.com/10388108/diff/6/content/common/gpu/med... content/common/gpu/media/video_decode_accelerator_unittest.cc:79: // AAVC data required to initialize the H.264 video decoder. On 2012/05/15 17:55:22, Ami Fischman wrote: > I don't know what this is, but: > 1) can it move to live near its only use? This is a part of the mp4 movie. It stores information about the H.264 sequence. I think it makes sense to put this next to test_video_data but I can move it where it's used if you like. > 2) can it move into vda_mac.mm or does it really need to be set from the > outside? If the latter, is this going to need to be plumbed from ppapi through > the various IPC layers? I'm planning to plumb this through the the ppapi IPC layer in later CLs. > 3) where do the values come from / what do they mean? I got these values from ffmpeg by converting test-25fps.h264 to a .mp4 movie then getting the value of AVCodecContext::extradata. https://chromiumcodereview.appspot.com/10388108/diff/6/content/common/gpu/med... content/common/gpu/media/video_decode_accelerator_unittest.cc:751: #if 0 On 2012/05/15 17:55:22, Ami Fischman wrote: > Blort? Oops, removed. https://chromiumcodereview.appspot.com/10388108/diff/6/content/common/gpu/med... content/common/gpu/media/video_decode_accelerator_unittest.cc:840: DXVAVideoDecodeAccelerator::PreSandboxInitialization(); On 2012/05/15 17:55:22, Ami Fischman wrote: > why not move this into the OS_WIN branch of > RenderingHelper::InitializePlatform() as well? The RenderingHelper abstracts the EGL stuff. It doesn't currently depend on anything decoder related. Also, I'll have to add a similar function for OSX. https://chromiumcodereview.appspot.com/10388108/diff/6/content/content_tests.... File content/content_tests.gypi (right): https://chromiumcodereview.appspot.com/10388108/diff/6/content/content_tests.... content/content_tests.gypi:485: 'target_name': 'video_decode_accelerator_mac_unittest', On 2012/05/15 17:55:22, Ami Fischman wrote: > Having a target name per platform was an organic mistake (made sense when > omx_vda was the only VDA); s/_mac// here to stop propagating that mistake? Done. https://chromiumcodereview.appspot.com/10388108/diff/6/content/content_tests.... content/content_tests.gypi:487: '../media/media.gyp:media', On 2012/05/15 17:55:22, Ami Fischman wrote: > unnecessary Done.
FYIs: +cc posciak, author of VAVDA (https://chromiumcodereview.appspot.com/9814001/) +cc iyengar, author of DXVA
Turns out that CR comments are like pringles, I can't stop at just one. Don't be alarmed by the volume here - they should mostly be easy to deal with. https://chromiumcodereview.appspot.com/10388108/diff/5010/base/mac/scoped_cft... File base/mac/scoped_cftyperef.h (right): https://chromiumcodereview.appspot.com/10388108/diff/5010/base/mac/scoped_cft... base/mac/scoped_cftyperef.h:1: // Copyright (c) 2010 The Chromium Authors. All rights reserved. Copyright year needs updating. https://chromiumcodereview.appspot.com/10388108/diff/5010/base/mac/scoped_cft... base/mac/scoped_cftyperef.h:1: // Copyright (c) 2010 The Chromium Authors. All rights reserved. My comments on this file are purely FYI; you'll need an OWNERS review (Nico, I guess). https://chromiumcodereview.appspot.com/10388108/diff/5010/base/mac/scoped_cft... base/mac/scoped_cftyperef.h:2: // Use of this source code is governed by a BSD-style license that can be ...BUT, that said, this change seems not worth it to me. Without this change, the rest of your CL changes only in one line: info.image.reset(image, base::mac::RETAIN); becomes info.image.reset(CFRetain(image)); ISTM the latter is actually better (again, knowing zilch about objc idioms/conventions/style). https://chromiumcodereview.appspot.com/10388108/diff/5010/base/mac/scoped_cft... base/mac/scoped_cftyperef.h:17: enum OwnershipPolicy { Any reason not to live inside ScopedCFTypeRef? (in the interest of keeping the namespace slightly less polluted) https://chromiumcodereview.appspot.com/10388108/diff/5010/base/mac/scoped_cft... base/mac/scoped_cftyperef.h:31: // CFRetain(). This paragraph seems like the doco for ASSUME and at least half of its assertions are incorrect for RETAIN :) https://chromiumcodereview.appspot.com/10388108/diff/5010/base/mac/scoped_cft... base/mac/scoped_cftyperef.h:38: OwnershipPolicy policy = ASSUME) I don't know that this style-guide violation (default param value) is justified by the compatibility sentence at l.24-25... https://chromiumcodereview.appspot.com/10388108/diff/5010/content/common/gpu/... File content/common/gpu/media/mac_video_decode_accelerator.h (right): https://chromiumcodereview.appspot.com/10388108/diff/5010/content/common/gpu/... content/common/gpu/media/mac_video_decode_accelerator.h:26: class MacVideoDecodeAccelerator : public media::VideoDecodeAccelerator { Impl has a refreshing lack of mention of threads. If everything in this class is really running in a single thread (yay!) then can you make this NotThreadSafe and assert same-threadedness in all the entry points (public methods & base::Bind()'d methods not posted to ML::current())? Otherwise please doco (& assert) which functions run on which threads. https://chromiumcodereview.appspot.com/10388108/diff/5010/content/common/gpu/... content/common/gpu/media/mac_video_decode_accelerator.h:71: scoped_refptr<gfx::VideoDecodeAccelerationSupport> vda_; Does VDAS need to be ref-counted (as opposed to not, and stored in a scoped_ptr<>)? https://chromiumcodereview.appspot.com/10388108/diff/5010/content/common/gpu/... content/common/gpu/media/mac_video_decode_accelerator.h:71: scoped_refptr<gfx::VideoDecodeAccelerationSupport> vda_; Ouch; the name "vda_" makes me think of the interface *this* class implements, not a support class. Rename to vda_support_? https://chromiumcodereview.appspot.com/10388108/diff/5010/content/common/gpu/... content/common/gpu/media/mac_video_decode_accelerator.h:77: struct PendingPictureInfo { s/pending/used/g (here and below) to parallel "available"_pictures_? https://chromiumcodereview.appspot.com/10388108/diff/5010/content/common/gpu/... content/common/gpu/media/mac_video_decode_accelerator.h:87: DecodedImageInfo(); unnecessary? https://chromiumcodereview.appspot.com/10388108/diff/5010/content/common/gpu/... content/common/gpu/media/mac_video_decode_accelerator.h:101: uint32_t frame_width_; unsigned is the devil. In this case use gfx::Size for the dimensions and avoid the argument ;) https://chromiumcodereview.appspot.com/10388108/diff/5010/content/common/gpu/... File content/common/gpu/media/mac_video_decode_accelerator.mm (right): https://chromiumcodereview.appspot.com/10388108/diff/5010/content/common/gpu/... content/common/gpu/media/mac_video_decode_accelerator.mm:21: class ScopedContextSetter { This seems like kind of overkill considering you use it exactly once... (esp. given you need did_succeed()) https://chromiumcodereview.appspot.com/10388108/diff/5010/content/common/gpu/... content/common/gpu/media/mac_video_decode_accelerator.mm:53: IOSurfaceSupport* io_surface_support = IOSurfaceSupport::Initialize(); Get & check this before Initialize returns true? https://chromiumcodereview.appspot.com/10388108/diff/5010/content/common/gpu/... content/common/gpu/media/mac_video_decode_accelerator.mm:59: if (!io_surface) When can this happen? Do you want to log something about it? Do you want to abort the decode because of it? Pretty much the same question applies to every unhappy codepath below (early return from void functions, return false from bool functions). For an example of what I mean, see the RETURN_ON_{,OMX_}FAILURE macros in OVDA: http://code.google.com/searchframe#OAMlx_jo-ck/src/content/common/gpu/media/o... https://chromiumcodereview.appspot.com/10388108/diff/5010/content/common/gpu/... content/common/gpu/media/mac_video_decode_accelerator.mm:76: glBindTexture(GL_TEXTURE_RECTANGLE_ARB, 0); Why this line & the next? https://chromiumcodereview.appspot.com/10388108/diff/5010/content/common/gpu/... content/common/gpu/media/mac_video_decode_accelerator.mm:78: if (glGetError() != GL_NO_ERROR) if (foo != bar) return false; return true; is more concisely written as: return foo == bar; https://chromiumcodereview.appspot.com/10388108/diff/5010/content/common/gpu/... content/common/gpu/media/mac_video_decode_accelerator.mm:95: cgl_context_ = static_cast<CGLContextObj>(gl_context); Why not type the param explicitly and avoid the cast here? https://chromiumcodereview.appspot.com/10388108/diff/5010/content/common/gpu/... content/common/gpu/media/mac_video_decode_accelerator.mm:98: bool MacVideoDecodeAccelerator::SetConfigInfo( This at least needs a TODO to support mid-stream frame size changes (and test that failure is graceful, not resulting in ASAN violations & security bugs). https://chromiumcodereview.appspot.com/10388108/diff/5010/content/common/gpu/... content/common/gpu/media/mac_video_decode_accelerator.mm:111: if (status != gfx::VideoDecodeAccelerationSupport::VDA_SUCCESS) That's a funny name for the enum given it's already scoped inside VDAS. I'd drop the VDA_. https://chromiumcodereview.appspot.com/10388108/diff/5010/content/common/gpu/... content/common/gpu/media/mac_video_decode_accelerator.mm:111: if (status != gfx::VideoDecodeAccelerationSupport::VDA_SUCCESS) again shorter/clearer as: return status == gfx::VideoDecodeAccelerationSupport::VDA_SUCCESS; https://chromiumcodereview.appspot.com/10388108/diff/5010/content/common/gpu/... content/common/gpu/media/mac_video_decode_accelerator.mm:118: client_->NotifyInitializeDone(); IIRC this can't be done in-line b/c of reentrancy in the caller. ML::current()->PostTask() it instead. https://chromiumcodereview.appspot.com/10388108/diff/5010/content/common/gpu/... content/common/gpu/media/mac_video_decode_accelerator.mm:126: CHECK(false); This isn't very cool ;) https://chromiumcodereview.appspot.com/10388108/diff/5010/content/common/gpu/... content/common/gpu/media/mac_video_decode_accelerator.mm:131: if (buffer_size < nalu_len_field_size_ + 1) { Why so forgiving? Shouldn't this be an invalid-stream error? https://chromiumcodereview.appspot.com/10388108/diff/5010/content/common/gpu/... content/common/gpu/media/mac_video_decode_accelerator.mm:137: // The decoder can only handle slice types (1-5). Drop the parens? https://chromiumcodereview.appspot.com/10388108/diff/5010/content/common/gpu/... content/common/gpu/media/mac_video_decode_accelerator.mm:140: if (nalu_type < 1 || nalu_type > 5) { are you sure about these bounds? H264Parser claims these are the slice types: kPSlice = 0, kBSlice = 1, kISlice = 2, kSPSlice = 3, kSISlice = 4, https://chromiumcodereview.appspot.com/10388108/diff/5010/content/common/gpu/... content/common/gpu/media/mac_video_decode_accelerator.mm:147: std::vector<uint8_t> vector(buffer, buffer + buffer_size); That's quite the variable name.... vbuffer? https://chromiumcodereview.appspot.com/10388108/diff/5010/content/common/gpu/... content/common/gpu/media/mac_video_decode_accelerator.mm:157: } Do you want to have any kind of error checking? I.e. that frame_buffer_size actually fits inside nalu_len_field_size_ bytes ? https://chromiumcodereview.appspot.com/10388108/diff/5010/content/common/gpu/... content/common/gpu/media/mac_video_decode_accelerator.mm:181: DCHECK(it != pending_pictures_.end()); Do you really want to crash the GPU process if a buggy PPAPI plugin provides an incorrect id? https://chromiumcodereview.appspot.com/10388108/diff/5010/content/common/gpu/... content/common/gpu/media/mac_video_decode_accelerator.mm:202: vda_ = NULL; Don't all the other entry points want to check for vda_ being NULL before assuming it isn't? https://chromiumcodereview.appspot.com/10388108/diff/5010/content/common/gpu/... content/common/gpu/media/mac_video_decode_accelerator.mm:206: MacVideoDecodeAccelerator::~MacVideoDecodeAccelerator() { move to below ctor call Destroy()? https://chromiumcodereview.appspot.com/10388108/diff/5010/content/common/gpu/... content/common/gpu/media/mac_video_decode_accelerator.mm:214: if (image) { how can this fail? https://chromiumcodereview.appspot.com/10388108/diff/5010/content/common/gpu/... content/common/gpu/media/mac_video_decode_accelerator.mm:222: client_->NotifyEndOfBitstreamBuffer(bitstream_buffer_id); This is wrong if multiple frames are encoded in a single bitstreambuffer. Does VDAS need to be extended to notify when a buffer is fully consumed? https://chromiumcodereview.appspot.com/10388108/diff/5010/content/common/gpu/... content/common/gpu/media/mac_video_decode_accelerator.mm:225: void MacVideoDecodeAccelerator::SendImages() { This sends at most one image, so the name is misleading. I think if you replace the if at the top of the method with while (available_pictures_.size() && decoded_images_.size()) and put the rest of the function inside that while loop, you'll be good. https://chromiumcodereview.appspot.com/10388108/diff/5010/content/common/gpu/... content/common/gpu/media/mac_video_decode_accelerator.mm:239: available_pictures_.pop_front(); move this up to right below the .front() call? https://chromiumcodereview.appspot.com/10388108/diff/5010/content/common/gpu/... content/common/gpu/media/mac_video_decode_accelerator.mm:240: PendingPictureInfo pending_info(picture_buffer); why use the intermediate? https://chromiumcodereview.appspot.com/10388108/diff/5010/content/common/gpu/... content/common/gpu/media/mac_video_decode_accelerator.mm:241: pending_info.image = info.image; why not pass both buffer & image to the PPI ctor? (and make them const in PPI) https://chromiumcodereview.appspot.com/10388108/diff/5010/content/common/gpu/... content/common/gpu/media/mac_video_decode_accelerator.mm:242: pending_pictures_.insert(std::pair<int, PendingPictureInfo>( std::make_pair lets you drop the template params. https://chromiumcodereview.appspot.com/10388108/diff/5010/content/common/gpu/... content/common/gpu/media/mac_video_decode_accelerator.mm:245: if (client_) If !client_ early-return at the top of the function? https://chromiumcodereview.appspot.com/10388108/diff/5010/content/common/gpu/... content/common/gpu/media/mac_video_decode_accelerator.mm:246: client_->PictureReady(picture); can construct this inline: client_->PictureReady(media::Picture(picture_buffer.id(), info.bitstream_buffer_id)); if you like. https://chromiumcodereview.appspot.com/10388108/diff/5010/content/common/gpu/... content/common/gpu/media/mac_video_decode_accelerator.mm:250: if (client_) assert decoded_images_ is empty? https://chromiumcodereview.appspot.com/10388108/diff/5010/content/common/gpu/... content/common/gpu/media/mac_video_decode_accelerator.mm:262: : picture_buffer(pic.id(), pic.size(), pic.texture_id()) { what's wrong w/ the copy ctor? https://chromiumcodereview.appspot.com/10388108/diff/5010/content/common/gpu/... File content/common/gpu/media/rendering_helper_egl.cc (right): https://chromiumcodereview.appspot.com/10388108/diff/5010/content/common/gpu/... content/common/gpu/media/rendering_helper_egl.cc:201: ); I realize you did this on rsesek's suggestion, but I don't think it's defensible in terms of the style guide. My memory is vague, but is the trailing \n following the last } actually necessary? Because of it isn't, the most styleguidey way of doing this would be to close with: gl_FragColor = texture2D(tex, interp_tc); }); (the only reason I'm bothering to comment on this is that the previous indentation is the only reasonable thing for an editor to auto-indent to, and it's nice not having to correct your editor's auto-indent). https://chromiumcodereview.appspot.com/10388108/diff/5010/content/common/gpu/... File content/common/gpu/media/rendering_helper_mac.mm (right): https://chromiumcodereview.appspot.com/10388108/diff/5010/content/common/gpu/... content/common/gpu/media/rendering_helper_mac.mm:146: CHECK_EQ(1, num_windows); generally order is: variable, constant https://chromiumcodereview.appspot.com/10388108/diff/5010/content/common/gpu/... File content/common/gpu/media/video_decode_accelerator_unittest.cc (right): https://chromiumcodereview.appspot.com/10388108/diff/5010/content/common/gpu/... content/common/gpu/media/video_decode_accelerator_unittest.cc:80: const uint8_t MP4_EXTRA_DATA[] = { Hopefully you can drop this in favor of H264Parser, per my response https://chromiumcodereview.appspot.com/10408003/#msg3 https://chromiumcodereview.appspot.com/10388108/diff/5010/content/common/gpu/... content/common/gpu/media/video_decode_accelerator_unittest.cc:548: DeleteDecoder(); I think you missed my comment: Should this be posting instead of calling in-line to account for outstanding Decode tasks from above, now? https://chromiumcodereview.appspot.com/10388108/diff/5010/content/content_tes... File content/content_tests.gypi (right): https://chromiumcodereview.appspot.com/10388108/diff/5010/content/content_tes... content/content_tests.gypi:477: 'target_name': 'video_decode_accelerator_unittest', Sorry, I was overly terse before. The reason the naming mistake hasn't been fixed yet is that it's tricky - the name omx_video_decode_accelerator_unittest is referenced in other places (cros stuff) so can't be changed this easily. I was just saying that for the new target you should use the generic name instead of propagating my mistake further.
Hey Ami. Thanks so much for the thorough review. I think I've addressed all your comments in this patch. Thanks! https://chromiumcodereview.appspot.com/10388108/diff/5010/base/mac/scoped_cft... File base/mac/scoped_cftyperef.h (right): https://chromiumcodereview.appspot.com/10388108/diff/5010/base/mac/scoped_cft... base/mac/scoped_cftyperef.h:1: // Copyright (c) 2010 The Chromium Authors. All rights reserved. On 2012/05/17 21:44:47, Ami Fischman wrote: > Copyright year needs updating. Done. https://chromiumcodereview.appspot.com/10388108/diff/5010/base/mac/scoped_cft... base/mac/scoped_cftyperef.h:2: // Use of this source code is governed by a BSD-style license that can be On 2012/05/17 21:44:47, Ami Fischman wrote: > ...BUT, that said, this change seems not worth it to me. > Without this change, the rest of your CL changes only in one line: > info.image.reset(image, base::mac::RETAIN); > becomes > info.image.reset(CFRetain(image)); > > ISTM the latter is actually better (again, knowing zilch about objc > idioms/conventions/style). The other thing I need here is the copy constructor and assignment operator. This makes it easy to store the object in a map and vector. https://chromiumcodereview.appspot.com/10388108/diff/5010/base/mac/scoped_cft... base/mac/scoped_cftyperef.h:17: enum OwnershipPolicy { On 2012/05/17 21:44:47, Ami Fischman wrote: > Any reason not to live inside ScopedCFTypeRef? > (in the interest of keeping the namespace slightly less polluted) Moving this would make the calling code look like this: info.image.reset(image, base::mac::ScopedCFTypeRef<CVImageBufferRef>::RETAIN) which seems too verbose to me. https://chromiumcodereview.appspot.com/10388108/diff/5010/base/mac/scoped_cft... base/mac/scoped_cftyperef.h:31: // CFRetain(). On 2012/05/17 21:44:47, Ami Fischman wrote: > This paragraph seems like the doco for ASSUME and at least half of its > assertions are incorrect for RETAIN :) Done. https://chromiumcodereview.appspot.com/10388108/diff/5010/base/mac/scoped_cft... base/mac/scoped_cftyperef.h:38: OwnershipPolicy policy = ASSUME) On 2012/05/17 21:44:47, Ami Fischman wrote: > I don't know that this style-guide violation (default param value) is justified > by the compatibility sentence at l.24-25... This matches the scoped_nsprotocol constructor. If you think this should really change then mind if I do it a separate CL? https://chromiumcodereview.appspot.com/10388108/diff/5010/content/common/gpu/... File content/common/gpu/media/mac_video_decode_accelerator.h (right): https://chromiumcodereview.appspot.com/10388108/diff/5010/content/common/gpu/... content/common/gpu/media/mac_video_decode_accelerator.h:26: class MacVideoDecodeAccelerator : public media::VideoDecodeAccelerator { On 2012/05/17 21:44:47, Ami Fischman wrote: > Impl has a refreshing lack of mention of threads. If everything in this class > is really running in a single thread (yay!) then can you make this NotThreadSafe > and assert same-threadedness in all the entry points (public methods & > base::Bind()'d methods not posted to ML::current())? > Otherwise please doco (& assert) which functions run on which threads. Done. Ahh, sorry, forgot to comment on your previous comment about this issue. The class is single threaded. I changed the class to derive from base::NonThreadSafe. https://chromiumcodereview.appspot.com/10388108/diff/5010/content/common/gpu/... content/common/gpu/media/mac_video_decode_accelerator.h:71: scoped_refptr<gfx::VideoDecodeAccelerationSupport> vda_; On 2012/05/17 21:44:47, Ami Fischman wrote: > Ouch; the name "vda_" makes me think of the interface *this* class implements, > not a support class. Rename to vda_support_? Done. https://chromiumcodereview.appspot.com/10388108/diff/5010/content/common/gpu/... content/common/gpu/media/mac_video_decode_accelerator.h:71: scoped_refptr<gfx::VideoDecodeAccelerationSupport> vda_; On 2012/05/17 21:44:47, Ami Fischman wrote: > Does VDAS need to be ref-counted (as opposed to not, and stored in a > scoped_ptr<>)? > Yea, this needs to be refcounted. It's referenced by the Mac API so it can potentially live on a little longer than this class. https://chromiumcodereview.appspot.com/10388108/diff/5010/content/common/gpu/... content/common/gpu/media/mac_video_decode_accelerator.h:77: struct PendingPictureInfo { On 2012/05/17 21:44:47, Ami Fischman wrote: > s/pending/used/g (here and below) to parallel "available"_pictures_? Done. https://chromiumcodereview.appspot.com/10388108/diff/5010/content/common/gpu/... content/common/gpu/media/mac_video_decode_accelerator.h:87: DecodedImageInfo(); On 2012/05/17 21:44:47, Ami Fischman wrote: > unnecessary? Because of DecodedImageInfo::image the compiler complains that this is a complex struct and needs an out-of-line constructor and destructor. https://chromiumcodereview.appspot.com/10388108/diff/5010/content/common/gpu/... content/common/gpu/media/mac_video_decode_accelerator.h:101: uint32_t frame_width_; On 2012/05/17 21:44:47, Ami Fischman wrote: > unsigned is the devil. > In this case use gfx::Size for the dimensions and avoid the argument ;) Done. https://chromiumcodereview.appspot.com/10388108/diff/5010/content/common/gpu/... File content/common/gpu/media/mac_video_decode_accelerator.mm (right): https://chromiumcodereview.appspot.com/10388108/diff/5010/content/common/gpu/... content/common/gpu/media/mac_video_decode_accelerator.mm:21: class ScopedContextSetter { On 2012/05/17 21:44:47, Ami Fischman wrote: > This seems like kind of overkill considering you use it exactly once... > (esp. given you need did_succeed()) I agree that this is pretty verbose but I really like the simplicity. https://chromiumcodereview.appspot.com/10388108/diff/5010/content/common/gpu/... content/common/gpu/media/mac_video_decode_accelerator.mm:53: IOSurfaceSupport* io_surface_support = IOSurfaceSupport::Initialize(); On 2012/05/17 21:44:47, Ami Fischman wrote: > Get & check this before Initialize returns true? Done. https://chromiumcodereview.appspot.com/10388108/diff/5010/content/common/gpu/... content/common/gpu/media/mac_video_decode_accelerator.mm:59: if (!io_surface) On 2012/05/17 21:44:47, Ami Fischman wrote: > When can this happen? Do you want to log something about it? Do you want to > abort the decode because of it? > > Pretty much the same question applies to every unhappy codepath below (early > return from void functions, return false from bool functions). > > For an example of what I mean, see the RETURN_ON_{,OMX_}FAILURE macros in OVDA: > http://code.google.com/searchframe#OAMlx_jo-ck/src/content/common/gpu/media/o... Done. Added logging. https://chromiumcodereview.appspot.com/10388108/diff/5010/content/common/gpu/... content/common/gpu/media/mac_video_decode_accelerator.mm:76: glBindTexture(GL_TEXTURE_RECTANGLE_ARB, 0); On 2012/05/17 21:44:47, Ami Fischman wrote: > Why this line & the next? Hm.. it looks like we don't normally clear this in the chrome code. Removed. https://chromiumcodereview.appspot.com/10388108/diff/5010/content/common/gpu/... content/common/gpu/media/mac_video_decode_accelerator.mm:78: if (glGetError() != GL_NO_ERROR) On 2012/05/17 21:44:47, Ami Fischman wrote: > if (foo != bar) > return false; > return true; > > is more concisely written as: > return foo == bar; Done. https://chromiumcodereview.appspot.com/10388108/diff/5010/content/common/gpu/... content/common/gpu/media/mac_video_decode_accelerator.mm:95: cgl_context_ = static_cast<CGLContextObj>(gl_context); On 2012/05/17 21:44:47, Ami Fischman wrote: > Why not type the param explicitly and avoid the cast here? gfx::GLContext::GetHandle() returns void*. It seems better to do the cast in this platform specific file then in GpuVideoDecodeAccelerator which is cross platform. https://chromiumcodereview.appspot.com/10388108/diff/5010/content/common/gpu/... content/common/gpu/media/mac_video_decode_accelerator.mm:98: bool MacVideoDecodeAccelerator::SetConfigInfo( On 2012/05/17 21:44:47, Ami Fischman wrote: > This at least needs a TODO to support mid-stream frame size changes (and test > that failure is graceful, not resulting in ASAN violations & security bugs). I'm not sure if this config info is related to mid-stream frame size changes. https://chromiumcodereview.appspot.com/10388108/diff/5010/content/common/gpu/... content/common/gpu/media/mac_video_decode_accelerator.mm:111: if (status != gfx::VideoDecodeAccelerationSupport::VDA_SUCCESS) On 2012/05/17 21:44:47, Ami Fischman wrote: > again shorter/clearer as: > return status == gfx::VideoDecodeAccelerationSupport::VDA_SUCCESS; Done. https://chromiumcodereview.appspot.com/10388108/diff/5010/content/common/gpu/... content/common/gpu/media/mac_video_decode_accelerator.mm:111: if (status != gfx::VideoDecodeAccelerationSupport::VDA_SUCCESS) On 2012/05/17 21:44:47, Ami Fischman wrote: > That's a funny name for the enum given it's already scoped inside VDAS. I'd > drop the VDA_. Done. https://chromiumcodereview.appspot.com/10388108/diff/5010/content/common/gpu/... content/common/gpu/media/mac_video_decode_accelerator.mm:118: client_->NotifyInitializeDone(); On 2012/05/17 21:44:47, Ami Fischman wrote: > IIRC this can't be done in-line b/c of reentrancy in the caller. > ML::current()->PostTask() it instead. Done. https://chromiumcodereview.appspot.com/10388108/diff/5010/content/common/gpu/... content/common/gpu/media/mac_video_decode_accelerator.mm:126: CHECK(false); On 2012/05/17 21:44:47, Ami Fischman wrote: > This isn't very cool ;) Done. https://chromiumcodereview.appspot.com/10388108/diff/5010/content/common/gpu/... content/common/gpu/media/mac_video_decode_accelerator.mm:131: if (buffer_size < nalu_len_field_size_ + 1) { On 2012/05/17 21:44:47, Ami Fischman wrote: > Why so forgiving? Shouldn't this be an invalid-stream error? Done. https://chromiumcodereview.appspot.com/10388108/diff/5010/content/common/gpu/... content/common/gpu/media/mac_video_decode_accelerator.mm:137: // The decoder can only handle slice types (1-5). On 2012/05/17 21:44:47, Ami Fischman wrote: > Drop the parens? Done. https://chromiumcodereview.appspot.com/10388108/diff/5010/content/common/gpu/... content/common/gpu/media/mac_video_decode_accelerator.mm:140: if (nalu_type < 1 || nalu_type > 5) { On 2012/05/17 21:44:47, Ami Fischman wrote: > are you sure about these bounds? H264Parser claims these are the slice types: > > kPSlice = 0, > kBSlice = 1, > kISlice = 2, > kSPSlice = 3, > kSISlice = 4, Yea, the bounds are correct. 0 is unspecified which the Mac decoder rejects. https://chromiumcodereview.appspot.com/10388108/diff/5010/content/common/gpu/... content/common/gpu/media/mac_video_decode_accelerator.mm:147: std::vector<uint8_t> vector(buffer, buffer + buffer_size); On 2012/05/17 21:44:47, Ami Fischman wrote: > That's quite the variable name.... vbuffer? Done. https://chromiumcodereview.appspot.com/10388108/diff/5010/content/common/gpu/... content/common/gpu/media/mac_video_decode_accelerator.mm:157: } On 2012/05/17 21:44:47, Ami Fischman wrote: > Do you want to have any kind of error checking? > I.e. that frame_buffer_size actually fits inside nalu_len_field_size_ bytes ? Done. https://chromiumcodereview.appspot.com/10388108/diff/5010/content/common/gpu/... content/common/gpu/media/mac_video_decode_accelerator.mm:181: DCHECK(it != pending_pictures_.end()); On 2012/05/17 21:44:47, Ami Fischman wrote: > Do you really want to crash the GPU process if a buggy PPAPI plugin provides an > incorrect id? Done. https://chromiumcodereview.appspot.com/10388108/diff/5010/content/common/gpu/... content/common/gpu/media/mac_video_decode_accelerator.mm:202: vda_ = NULL; On 2012/05/17 21:44:47, Ami Fischman wrote: > Don't all the other entry points want to check for vda_ being NULL before > assuming it isn't? Done. https://chromiumcodereview.appspot.com/10388108/diff/5010/content/common/gpu/... content/common/gpu/media/mac_video_decode_accelerator.mm:206: MacVideoDecodeAccelerator::~MacVideoDecodeAccelerator() { On 2012/05/17 21:44:47, Ami Fischman wrote: > move to below ctor I'd rather leave it here to match the declaration order. > call Destroy()? Done. https://chromiumcodereview.appspot.com/10388108/diff/5010/content/common/gpu/... content/common/gpu/media/mac_video_decode_accelerator.mm:214: if (image) { On 2012/05/17 21:44:47, Ami Fischman wrote: > how can this fail? The decoder can send a NULL image if it failed to decode it for some reason. https://chromiumcodereview.appspot.com/10388108/diff/5010/content/common/gpu/... content/common/gpu/media/mac_video_decode_accelerator.mm:222: client_->NotifyEndOfBitstreamBuffer(bitstream_buffer_id); On 2012/05/17 21:44:47, Ami Fischman wrote: > This is wrong if multiple frames are encoded in a single bitstreambuffer. Does > VDAS need to be extended to notify when a buffer is fully consumed? The Mac API can only handle a single NALU at a time so I wrote this code assuming that each Decode() call only contains a single NALU. If we go with the CL that uses the content::H265Parser class then it should be fairly easy to extend this to handle multiple NALUs. Mind if fix this after that CL has landed. https://chromiumcodereview.appspot.com/10388108/diff/5010/content/common/gpu/... content/common/gpu/media/mac_video_decode_accelerator.mm:225: void MacVideoDecodeAccelerator::SendImages() { On 2012/05/17 21:44:47, Ami Fischman wrote: > This sends at most one image, so the name is misleading. > I think if you replace the if at the top of the method with > while (available_pictures_.size() && decoded_images_.size()) > and put the rest of the function inside that while loop, you'll be good. Done. https://chromiumcodereview.appspot.com/10388108/diff/5010/content/common/gpu/... content/common/gpu/media/mac_video_decode_accelerator.mm:239: available_pictures_.pop_front(); On 2012/05/17 21:44:47, Ami Fischman wrote: > move this up to right below the .front() call? Done. https://chromiumcodereview.appspot.com/10388108/diff/5010/content/common/gpu/... content/common/gpu/media/mac_video_decode_accelerator.mm:240: PendingPictureInfo pending_info(picture_buffer); On 2012/05/17 21:44:47, Ami Fischman wrote: > why use the intermediate? Done. https://chromiumcodereview.appspot.com/10388108/diff/5010/content/common/gpu/... content/common/gpu/media/mac_video_decode_accelerator.mm:241: pending_info.image = info.image; On 2012/05/17 21:44:47, Ami Fischman wrote: > why not pass both buffer & image to the PPI ctor? > (and make them const in PPI) Done. https://chromiumcodereview.appspot.com/10388108/diff/5010/content/common/gpu/... content/common/gpu/media/mac_video_decode_accelerator.mm:242: pending_pictures_.insert(std::pair<int, PendingPictureInfo>( On 2012/05/17 21:44:47, Ami Fischman wrote: > std::make_pair lets you drop the template params. The Mac compiler doesn't seem to allow this. https://chromiumcodereview.appspot.com/10388108/diff/5010/content/common/gpu/... content/common/gpu/media/mac_video_decode_accelerator.mm:245: if (client_) On 2012/05/17 21:44:47, Ami Fischman wrote: > If !client_ early-return at the top of the function? Done. https://chromiumcodereview.appspot.com/10388108/diff/5010/content/common/gpu/... content/common/gpu/media/mac_video_decode_accelerator.mm:246: client_->PictureReady(picture); On 2012/05/17 21:44:47, Ami Fischman wrote: > can construct this inline: > client_->PictureReady(media::Picture(picture_buffer.id(), > info.bitstream_buffer_id)); > if you like. Done. https://chromiumcodereview.appspot.com/10388108/diff/5010/content/common/gpu/... content/common/gpu/media/mac_video_decode_accelerator.mm:250: if (client_) On 2012/05/17 21:44:47, Ami Fischman wrote: > assert decoded_images_ is empty? Hm... it's possible that there are no available images so the decoded images haven't been sent yet. (DXVAVideoDecodeAccelerator has the same issue). https://chromiumcodereview.appspot.com/10388108/diff/5010/content/common/gpu/... content/common/gpu/media/mac_video_decode_accelerator.mm:262: : picture_buffer(pic.id(), pic.size(), pic.texture_id()) { On 2012/05/17 21:44:47, Ami Fischman wrote: > what's wrong w/ the copy ctor? Done. https://chromiumcodereview.appspot.com/10388108/diff/5010/content/common/gpu/... File content/common/gpu/media/rendering_helper_egl.cc (right): https://chromiumcodereview.appspot.com/10388108/diff/5010/content/common/gpu/... content/common/gpu/media/rendering_helper_egl.cc:201: ); On 2012/05/17 21:44:47, Ami Fischman wrote: > I realize you did this on rsesek's suggestion, but I don't think it's defensible > in terms of the style guide. > My memory is vague, but is the trailing \n following the last } actually > necessary? Because of it isn't, the most styleguidey way of doing this would be > to close with: > gl_FragColor = texture2D(tex, interp_tc); > }); > > (the only reason I'm bothering to comment on this is that the previous > indentation is the only reasonable thing for an editor to auto-indent to, and > it's nice not having to correct your editor's auto-indent). Done. https://chromiumcodereview.appspot.com/10388108/diff/5010/content/common/gpu/... content/common/gpu/media/rendering_helper_egl.cc:201: ); On 2012/05/17 21:44:47, Ami Fischman wrote: > I realize you did this on rsesek's suggestion, but I don't think it's defensible > in terms of the style guide. > My memory is vague, but is the trailing \n following the last } actually > necessary? Because of it isn't, the most styleguidey way of doing this would be > to close with: > gl_FragColor = texture2D(tex, interp_tc); > }); > > (the only reason I'm bothering to comment on this is that the previous > indentation is the only reasonable thing for an editor to auto-indent to, and > it's nice not having to correct your editor's auto-indent). Done. https://chromiumcodereview.appspot.com/10388108/diff/5010/content/common/gpu/... File content/common/gpu/media/rendering_helper_mac.mm (right): https://chromiumcodereview.appspot.com/10388108/diff/5010/content/common/gpu/... content/common/gpu/media/rendering_helper_mac.mm:146: CHECK_EQ(1, num_windows); On 2012/05/17 21:44:47, Ami Fischman wrote: > generally order is: variable, constant Done. https://chromiumcodereview.appspot.com/10388108/diff/5010/content/content_tes... File content/content_tests.gypi (right): https://chromiumcodereview.appspot.com/10388108/diff/5010/content/content_tes... content/content_tests.gypi:477: 'target_name': 'video_decode_accelerator_unittest', On 2012/05/17 21:44:47, Ami Fischman wrote: > Sorry, I was overly terse before. > The reason the naming mistake hasn't been fixed yet is that it's tricky - the > name omx_video_decode_accelerator_unittest is referenced in other places (cros > stuff) so can't be changed this easily. I was just saying that for the new > target you should use the generic name instead of propagating my mistake > further. Done.
fixed some typos
https://chromiumcodereview.appspot.com/10388108/diff/5010/base/mac/scoped_cft... File base/mac/scoped_cftyperef.h (right): https://chromiumcodereview.appspot.com/10388108/diff/5010/base/mac/scoped_cft... base/mac/scoped_cftyperef.h:38: OwnershipPolicy policy = ASSUME) On 2012/05/23 23:02:29, sail wrote: > On 2012/05/17 21:44:47, Ami Fischman wrote: > > I don't know that this style-guide violation (default param value) is > justified > > by the compatibility sentence at l.24-25... > > This matches the scoped_nsprotocol constructor. > If you think this should really change then mind if I do it a separate CL? Like I said, my opinions about this file are low-priority advisory. If base/mac/OWNERS are pleased to approve then more power to you. https://chromiumcodereview.appspot.com/10388108/diff/5010/content/common/gpu/... File content/common/gpu/media/mac_video_decode_accelerator.mm (right): https://chromiumcodereview.appspot.com/10388108/diff/5010/content/common/gpu/... content/common/gpu/media/mac_video_decode_accelerator.mm:21: class ScopedContextSetter { On 2012/05/23 23:02:29, sail wrote: > On 2012/05/17 21:44:47, Ami Fischman wrote: > > This seems like kind of overkill considering you use it exactly once... > > (esp. given you need did_succeed()) > > I agree that this is pretty verbose but I really like the simplicity. "simplicity" I don't think that word means what you think it means ;) https://chromiumcodereview.appspot.com/10388108/diff/5010/content/common/gpu/... content/common/gpu/media/mac_video_decode_accelerator.mm:95: cgl_context_ = static_cast<CGLContextObj>(gl_context); On 2012/05/23 23:02:29, sail wrote: > On 2012/05/17 21:44:47, Ami Fischman wrote: > > Why not type the param explicitly and avoid the cast here? > > gfx::GLContext::GetHandle() returns void*. It seems better to do the cast in > this platform specific file then in GpuVideoDecodeAccelerator which is cross > platform. I disagree b/c the call-site is not cross-platform (it is in a platform-specific #ifdef). In fact the call-site is clearly cross-platform since it calls *this* file, which *is* platform specific :) https://chromiumcodereview.appspot.com/10388108/diff/25001/base/mac/scoped_cf... File base/mac/scoped_cftyperef.h (right): https://chromiumcodereview.appspot.com/10388108/diff/25001/base/mac/scoped_cf... base/mac/scoped_cftyperef.h:28: // constructor or in reset()) by taking over the caller's existing ownership s/ or/ or/ https://chromiumcodereview.appspot.com/10388108/diff/25001/content/common/gpu... File content/common/gpu/media/mac_video_decode_accelerator.h (right): https://chromiumcodereview.appspot.com/10388108/diff/25001/content/common/gpu... content/common/gpu/media/mac_video_decode_accelerator.h:86: media::PictureBuffer picture_buffer; Can this and the next field be const? https://chromiumcodereview.appspot.com/10388108/diff/25001/content/common/gpu... File content/common/gpu/media/mac_video_decode_accelerator.mm (right): https://chromiumcodereview.appspot.com/10388108/diff/25001/content/common/gpu... content/common/gpu/media/mac_video_decode_accelerator.mm:51: DLOG(ERROR) << "Unable to set OpenGL context."; s/DLOG(ERROR)/DVLOG(1)/ here and everywhere below (see chromium-dev traffic about why). https://chromiumcodereview.appspot.com/10388108/diff/25001/content/common/gpu... content/common/gpu/media/mac_video_decode_accelerator.mm:61: DLOG(ERROR) << "Unable to get IOSurface for CVPixelBuffer."; I'm afraid I wasn't clear before. The problem with the unhappy codepaths was only half missing logging. The bigger half of the problem is whether the failure should make the MacVDA instance unusable going forward, and a secondary concern is whether a particular failure should trigger client notification of error. Wrapping these three things (logging, setting error status, and notifying client) in a macro helps to make sure you never forget one or more of them. https://chromiumcodereview.appspot.com/10388108/diff/25001/content/common/gpu... content/common/gpu/media/mac_video_decode_accelerator.mm:97: uint32_t frame_width, On 2012/05/23 23:02:29, sail wrote: > On 2012/05/17 21:44:47, Ami Fischman wrote: > > This at least needs a TODO to support mid-stream frame size changes (and test > > that failure is graceful, not resulting in ASAN violations & security bugs). > > I'm not sure if this config info is related to mid-stream frame size changes. This config info includes width & height, and the mid-stream frame size changes I'm talking about are changes in width & height. How can they be unrelated? https://chromiumcodereview.appspot.com/10388108/diff/25001/content/common/gpu... content/common/gpu/media/mac_video_decode_accelerator.mm:98: uint32_t frame_height, Is your plan to land this first and then immediately land https://chromiumcodereview.appspot.com/10411085/ top of it, or what? https://chromiumcodereview.appspot.com/10388108/diff/25001/content/common/gpu... content/common/gpu/media/mac_video_decode_accelerator.mm:159: DCHECK(nalu_len_field_size_ <= 4); DCHECK_LE https://chromiumcodereview.appspot.com/10388108/diff/25001/content/common/gpu... content/common/gpu/media/mac_video_decode_accelerator.mm:160: uint64_t max_frame_buffer_size = (1llu << (nalu_len_field_size_ * 8)) - 1; s/uint64/int64/ s/llu/LL/ https://chromiumcodereview.appspot.com/10388108/diff/25001/content/common/gpu... content/common/gpu/media/mac_video_decode_accelerator.mm:161: if (frame_buffer_size > max_frame_buffer_size) { FWIW, writing this as if (frame_buffer_size >> (nalu_len_field_size_ * 8)) might be clearer. (if you keep m_f_b_s, make it const) https://chromiumcodereview.appspot.com/10388108/diff/25001/content/common/gpu... content/common/gpu/media/mac_video_decode_accelerator.mm:162: LOG(ERROR) << "Bitstream is too large."; s/is/buffer is/ https://chromiumcodereview.appspot.com/10388108/diff/25001/content/common/gpu... content/common/gpu/media/mac_video_decode_accelerator.mm:181: client_->ProvidePictureBuffers(kNumPictureBuffers, frame_size_); Again needs to be out-of-line to avoid re-entrancy. https://chromiumcodereview.appspot.com/10388108/diff/25001/content/common/gpu... content/common/gpu/media/mac_video_decode_accelerator.mm:212: vda_support_->Flush(true); I'm surprised this can be done synchronously. In particular it means that in the same stack where the client called Flush() it might receive PictureReady callbacks. I think this needs to be moved out-of-line. (basically replace NotifyFlushDone with DoFlush). https://chromiumcodereview.appspot.com/10388108/diff/25001/content/common/gpu... content/common/gpu/media/mac_video_decode_accelerator.mm:245: if (image) { Is this failing not a cause for notifying the client of something? https://chromiumcodereview.appspot.com/10388108/diff/25001/content/common/gpu... content/common/gpu/media/mac_video_decode_accelerator.mm:253: client_->NotifyEndOfBitstreamBuffer(bitstream_buffer_id); Add // TODO(sail): this assumes Decode() is handed a single NALU at a time. Make that assumption go away. https://chromiumcodereview.appspot.com/10388108/diff/25001/content/common/gpu... content/common/gpu/media/mac_video_decode_accelerator.mm:257: if (!client_) In this case do you not want to clear the queues? https://chromiumcodereview.appspot.com/10388108/diff/25001/content/common/gpu... content/common/gpu/media/mac_video_decode_accelerator.mm:273: used_pictures_.insert(std::pair<int, UsedPictureInfo>( what's the problem if you replace std::pair<int, UsedPictureInfo>( with std::make_pair( ? https://chromiumcodereview.appspot.com/10388108/diff/25001/content/common/gpu... content/common/gpu/media/mac_video_decode_accelerator.mm:287: client_->NotifyFlushDone(); On 2012/05/23 23:02:29, sail wrote: > On 2012/05/17 21:44:47, Ami Fischman wrote: > > assert decoded_images_ is empty? > > Hm... it's possible that there are no available images so the decoded images > haven't been sent yet. (DXVAVideoDecodeAccelerator has the same issue). In that case in what sense is the flush "done"??? https://chromiumcodereview.appspot.com/10388108/diff/25001/content/common/gpu... File content/common/gpu/media/video_decode_accelerator_unittest.cc (right): https://chromiumcodereview.appspot.com/10388108/diff/25001/content/common/gpu... content/common/gpu/media/video_decode_accelerator_unittest.cc:79: // AAVC data required to initialize the H.264 video decoder. Add TODO to drop this? https://chromiumcodereview.appspot.com/10388108/diff/25001/content/common/gpu... content/common/gpu/media/video_decode_accelerator_unittest.cc:548: DeleteDecoder(); I think you missed this comment from my last review: > Should this be posting instead of calling in-line to account for outstanding > Decode tasks from above, now?
https://chromiumcodereview.appspot.com/10388108/diff/5010/content/common/gpu/... File content/common/gpu/media/mac_video_decode_accelerator.mm (right): https://chromiumcodereview.appspot.com/10388108/diff/5010/content/common/gpu/... content/common/gpu/media/mac_video_decode_accelerator.mm:95: cgl_context_ = static_cast<CGLContextObj>(gl_context); On 2012/05/24 21:30:29, Ami Fischman wrote: > On 2012/05/23 23:02:29, sail wrote: > > On 2012/05/17 21:44:47, Ami Fischman wrote: > > > Why not type the param explicitly and avoid the cast here? > > > > gfx::GLContext::GetHandle() returns void*. It seems better to do the cast in > > this platform specific file then in GpuVideoDecodeAccelerator which is cross > > platform. > > I disagree b/c the call-site is not cross-platform (it is in a platform-specific > #ifdef). In fact the call-site is clearly cross-platform since it calls *this* > file, which *is* platform specific :) Done. https://chromiumcodereview.appspot.com/10388108/diff/25001/base/mac/scoped_cf... File base/mac/scoped_cftyperef.h (right): https://chromiumcodereview.appspot.com/10388108/diff/25001/base/mac/scoped_cf... base/mac/scoped_cftyperef.h:28: // constructor or in reset()) by taking over the caller's existing ownership On 2012/05/24 21:30:30, Ami Fischman wrote: > s/ or/ or/ Done. https://chromiumcodereview.appspot.com/10388108/diff/25001/content/common/gpu... File content/common/gpu/media/mac_video_decode_accelerator.h (right): https://chromiumcodereview.appspot.com/10388108/diff/25001/content/common/gpu... content/common/gpu/media/mac_video_decode_accelerator.h:86: media::PictureBuffer picture_buffer; On 2012/05/24 21:30:30, Ami Fischman wrote: > Can this and the next field be const? Done. https://chromiumcodereview.appspot.com/10388108/diff/25001/content/common/gpu... File content/common/gpu/media/mac_video_decode_accelerator.mm (right): https://chromiumcodereview.appspot.com/10388108/diff/25001/content/common/gpu... content/common/gpu/media/mac_video_decode_accelerator.mm:51: DLOG(ERROR) << "Unable to set OpenGL context."; On 2012/05/24 21:30:30, Ami Fischman wrote: > s/DLOG(ERROR)/DVLOG(1)/ > here and everywhere below (see chromium-dev traffic about why). Done. https://chromiumcodereview.appspot.com/10388108/diff/25001/content/common/gpu... content/common/gpu/media/mac_video_decode_accelerator.mm:61: DLOG(ERROR) << "Unable to get IOSurface for CVPixelBuffer."; On 2012/05/24 21:30:30, Ami Fischman wrote: > I'm afraid I wasn't clear before. The problem with the unhappy codepaths was > only half missing logging. The bigger half of the problem is whether the > failure should make the MacVDA instance unusable going forward, and a secondary > concern is whether a particular failure should trigger client notification of > error. > Wrapping these three things (logging, setting error status, and notifying > client) in a macro helps to make sure you never forget one or more of them. Done. To mark the MacVDA instance unusable I'm just clearing the vda_support_ member variable. This is simpler than adding a new state. https://chromiumcodereview.appspot.com/10388108/diff/25001/content/common/gpu... content/common/gpu/media/mac_video_decode_accelerator.mm:97: uint32_t frame_width, On 2012/05/24 21:30:30, Ami Fischman wrote: > On 2012/05/23 23:02:29, sail wrote: > > On 2012/05/17 21:44:47, Ami Fischman wrote: > > > This at least needs a TODO to support mid-stream frame size changes (and > test > > > that failure is graceful, not resulting in ASAN violations & security bugs). > > > > I'm not sure if this config info is related to mid-stream frame size changes. > > This config info includes width & height, and the mid-stream frame size changes > I'm talking about are changes in width & height. How can they be unrelated? So taking a step back. All the Mac video API client code that I've seen do this: VDACreate(extra_data) while (!end of data) { VDADecode(); } VDADestroy(); So if there's a frame size change in the middle of the stream I assume VDADecode() will either handle it or not handle it. Either way it doesn't seem like there's much I should be doing. Also, frame size is just one example of the information encoded in the AVC configuration record. If you can point me at a sample video I can test the Mac VDA API and see what it does. I've found that getting sample videos that the Mac video decoder can handle is tricky. It's very picky about what it supports. https://chromiumcodereview.appspot.com/10388108/diff/25001/content/common/gpu... content/common/gpu/media/mac_video_decode_accelerator.mm:98: uint32_t frame_height, On 2012/05/24 21:30:30, Ami Fischman wrote: > Is your plan to land this first and then immediately land > https://chromiumcodereview.appspot.com/10411085/ top of it, or what? I'm planning to land the various CLs one after another but as long as the ordering is correct they won't break the build. I'll hold off on hooking up the Mac VDA code to GpuVideoDecodeAccelerator till the very last CL. https://chromiumcodereview.appspot.com/10388108/diff/25001/content/common/gpu... content/common/gpu/media/mac_video_decode_accelerator.mm:159: DCHECK(nalu_len_field_size_ <= 4); On 2012/05/24 21:30:30, Ami Fischman wrote: > DCHECK_LE Done. https://chromiumcodereview.appspot.com/10388108/diff/25001/content/common/gpu... content/common/gpu/media/mac_video_decode_accelerator.mm:160: uint64_t max_frame_buffer_size = (1llu << (nalu_len_field_size_ * 8)) - 1; On 2012/05/24 21:30:30, Ami Fischman wrote: > s/uint64/int64/ > s/llu/LL/ Done. https://chromiumcodereview.appspot.com/10388108/diff/25001/content/common/gpu... content/common/gpu/media/mac_video_decode_accelerator.mm:161: if (frame_buffer_size > max_frame_buffer_size) { On 2012/05/24 21:30:30, Ami Fischman wrote: > FWIW, writing this as > if (frame_buffer_size >> (nalu_len_field_size_ * 8)) > might be clearer. > > (if you keep m_f_b_s, make it const) Unfortunately that overflows when nalu_len_field_size_ is 4 and chrome is built as a 32 bit app. Added const. https://chromiumcodereview.appspot.com/10388108/diff/25001/content/common/gpu... content/common/gpu/media/mac_video_decode_accelerator.mm:162: LOG(ERROR) << "Bitstream is too large."; On 2012/05/24 21:30:30, Ami Fischman wrote: > s/is/buffer is/ Done. https://chromiumcodereview.appspot.com/10388108/diff/25001/content/common/gpu... content/common/gpu/media/mac_video_decode_accelerator.mm:181: client_->ProvidePictureBuffers(kNumPictureBuffers, frame_size_); On 2012/05/24 21:30:30, Ami Fischman wrote: > Again needs to be out-of-line to avoid re-entrancy. Done. https://chromiumcodereview.appspot.com/10388108/diff/25001/content/common/gpu... content/common/gpu/media/mac_video_decode_accelerator.mm:212: vda_support_->Flush(true); On 2012/05/24 21:30:30, Ami Fischman wrote: > I'm surprised this can be done synchronously. > In particular it means that in the same stack where the client called Flush() it > might receive PictureReady callbacks. I think this needs to be moved > out-of-line. > (basically replace NotifyFlushDone with DoFlush). Even though the flush is synchronous the decoded images are queued up in the thread's message loop. The NotifyFlushDone is added to the end of the message loop so it gets called once all the images are sent back. https://chromiumcodereview.appspot.com/10388108/diff/25001/content/common/gpu... content/common/gpu/media/mac_video_decode_accelerator.mm:245: if (image) { On 2012/05/24 21:30:30, Ami Fischman wrote: > Is this failing not a cause for notifying the client of something? The image can be NULL if Reset() is called. I added a check for the status code though. https://chromiumcodereview.appspot.com/10388108/diff/25001/content/common/gpu... content/common/gpu/media/mac_video_decode_accelerator.mm:253: client_->NotifyEndOfBitstreamBuffer(bitstream_buffer_id); On 2012/05/24 21:30:30, Ami Fischman wrote: > Add > // TODO(sail): this assumes Decode() is handed a single NALU at a time. Make > that assumption go away. Done. https://chromiumcodereview.appspot.com/10388108/diff/25001/content/common/gpu... content/common/gpu/media/mac_video_decode_accelerator.mm:257: if (!client_) On 2012/05/24 21:30:30, Ami Fischman wrote: > In this case do you not want to clear the queues? I added code to clear the decoded image queue in Destroy() (which gets called when ever client is set to NULL). I also added a DCHECK here that if a client doesn't exist then the decoded image queue must be empty. https://chromiumcodereview.appspot.com/10388108/diff/25001/content/common/gpu... content/common/gpu/media/mac_video_decode_accelerator.mm:273: used_pictures_.insert(std::pair<int, UsedPictureInfo>( On 2012/05/24 21:30:30, Ami Fischman wrote: > what's the problem if you replace > std::pair<int, UsedPictureInfo>( > with > std::make_pair( > ? Done. https://chromiumcodereview.appspot.com/10388108/diff/25001/content/common/gpu... content/common/gpu/media/mac_video_decode_accelerator.mm:287: client_->NotifyFlushDone(); On 2012/05/24 21:30:30, Ami Fischman wrote: > On 2012/05/23 23:02:29, sail wrote: > > On 2012/05/17 21:44:47, Ami Fischman wrote: > > > assert decoded_images_ is empty? > > > > Hm... it's possible that there are no available images so the decoded images > > haven't been sent yet. (DXVAVideoDecodeAccelerator has the same issue). > > In that case in what sense is the flush "done"??? So it looks like there are a few solutions: #1 wait for the decoded images to be sent to the client (as new pictures become available). #2 give up and empty the decoded images queue #3 do nothing an send these images even after NotifyFlushDone is called. Do you have a preference for one vs the other? Also, it looks like I'd have to implement this for all three decoders. If you want me to do #1 can I do it in a separate CL? https://chromiumcodereview.appspot.com/10388108/diff/25001/content/common/gpu... File content/common/gpu/media/video_decode_accelerator_unittest.cc (right): https://chromiumcodereview.appspot.com/10388108/diff/25001/content/common/gpu... content/common/gpu/media/video_decode_accelerator_unittest.cc:79: // AAVC data required to initialize the H.264 video decoder. On 2012/05/24 21:30:30, Ami Fischman wrote: > Add TODO to drop this? Done. https://chromiumcodereview.appspot.com/10388108/diff/25001/content/common/gpu... content/common/gpu/media/video_decode_accelerator_unittest.cc:548: DeleteDecoder(); On 2012/05/24 21:30:30, Ami Fischman wrote: > I think you missed this comment from my last review: > > Should this be posting instead of calling in-line to account for outstanding > > Decode tasks from above, now? Ahh sorry, overlooked that. I reverted this change and updated the Mac code so that it no longer calls NotifyEndOfBitstreamBuffer() immediately.
https://chromiumcodereview.appspot.com/10388108/diff/25001/content/common/gpu... File content/common/gpu/media/mac_video_decode_accelerator.mm (right): https://chromiumcodereview.appspot.com/10388108/diff/25001/content/common/gpu... content/common/gpu/media/mac_video_decode_accelerator.mm:61: DLOG(ERROR) << "Unable to get IOSurface for CVPixelBuffer."; On 2012/05/29 03:45:01, sail wrote: > On 2012/05/24 21:30:30, Ami Fischman wrote: > > I'm afraid I wasn't clear before. The problem with the unhappy codepaths was > > only half missing logging. The bigger half of the problem is whether the > > failure should make the MacVDA instance unusable going forward, and a > secondary > > concern is whether a particular failure should trigger client notification of > > error. > > Wrapping these three things (logging, setting error status, and notifying > > client) in a macro helps to make sure you never forget one or more of them. > > Done. > > To mark the MacVDA instance unusable I'm just clearing the vda_support_ member > variable. This is simpler than adding a new state. Actually, I changed this to check for a client_ instead. This way it's compatible with my later CL that moves the vda_support_ creation into Decode().
https://chromiumcodereview.appspot.com/10388108/diff/25001/content/common/gpu... File content/common/gpu/media/mac_video_decode_accelerator.mm (right): https://chromiumcodereview.appspot.com/10388108/diff/25001/content/common/gpu... content/common/gpu/media/mac_video_decode_accelerator.mm:97: uint32_t frame_width, On 2012/05/29 03:45:01, sail wrote: > On 2012/05/24 21:30:30, Ami Fischman wrote: > > On 2012/05/23 23:02:29, sail wrote: > > > On 2012/05/17 21:44:47, Ami Fischman wrote: > > > > This at least needs a TODO to support mid-stream frame size changes (and > > test > > > > that failure is graceful, not resulting in ASAN violations & security > bugs). > > > > > > I'm not sure if this config info is related to mid-stream frame size > changes. > > > > This config info includes width & height, and the mid-stream frame size > changes > > I'm talking about are changes in width & height. How can they be unrelated? > > So taking a step back. > > All the Mac video API client code that I've seen do this: > VDACreate(extra_data) > while (!end of data) { > VDADecode(); > } > VDADestroy(); > > So if there's a frame size change in the middle of the stream I assume > VDADecode() will either handle it or not handle it. Either way it doesn't seem > like there's much I should be doing. Presumably you need to request different textures, no? > If you can point me at a sample video There's a mid-stream frame-size-changing .webm checked into third_party/WebKit/LayoutTests/platform/chromium/media/resources/frame_size_change.webm but that's webm. You'll have to transcode it to h264 to test. https://chromiumcodereview.appspot.com/10388108/diff/25001/content/common/gpu... content/common/gpu/media/mac_video_decode_accelerator.mm:161: if (frame_buffer_size > max_frame_buffer_size) { On 2012/05/29 03:45:01, sail wrote: > On 2012/05/24 21:30:30, Ami Fischman wrote: > > FWIW, writing this as > > if (frame_buffer_size >> (nalu_len_field_size_ * 8)) > > might be clearer. > > > > (if you keep m_f_b_s, make it const) > > Unfortunately that overflows when nalu_len_field_size_ is 4 and chrome is built > as a 32 bit app. WDYM "overflows"? https://chromiumcodereview.appspot.com/10388108/diff/25001/content/common/gpu... content/common/gpu/media/mac_video_decode_accelerator.mm:287: client_->NotifyFlushDone(); On 2012/05/29 03:45:01, sail wrote: > On 2012/05/24 21:30:30, Ami Fischman wrote: > > On 2012/05/23 23:02:29, sail wrote: > > > On 2012/05/17 21:44:47, Ami Fischman wrote: > > > > assert decoded_images_ is empty? > > > > > > Hm... it's possible that there are no available images so the decoded images > > > haven't been sent yet. (DXVAVideoDecodeAccelerator has the same issue). > > In that case in what sense is the flush "done"??? > So it looks like there are a few solutions: > #1 wait for the decoded images to be sent to the client (as new pictures > become available). This is the only option that's compatible with the API as described in video_decode_accelerator.h. > #2 give up and empty the decoded images queue That's a partial-Reset(), not a Flush(). > #3 do nothing an send these images even after NotifyFlushDone is called. This is terrible ;) A correct client would be within its rights to CHECK-fail. > Also, it looks like I'd have to implement this for all three decoders. Are you saying you think OVDA has this bug? Can you repro it in a unittest? (it shouldn't) > If you want me to do #1 can I do it in a separate CL? As long as it gets done in the short term, sure. https://chromiumcodereview.appspot.com/10388108/diff/32002/content/common/gpu... File content/common/gpu/media/mac_video_decode_accelerator.mm (right): https://chromiumcodereview.appspot.com/10388108/diff/32002/content/common/gpu... content/common/gpu/media/mac_video_decode_accelerator.mm:175: "Bitstream buffer is too large.", INVALID_ARGUMENT,); double space
http://codereview.chromium.org/10388108/diff/25001/content/common/gpu/media/m... File content/common/gpu/media/mac_video_decode_accelerator.mm (right): http://codereview.chromium.org/10388108/diff/25001/content/common/gpu/media/m... content/common/gpu/media/mac_video_decode_accelerator.mm:97: uint32_t frame_width, On 2012/05/30 00:29:58, Ami Fischman wrote: > On 2012/05/29 03:45:01, sail wrote: > > On 2012/05/24 21:30:30, Ami Fischman wrote: > > > On 2012/05/23 23:02:29, sail wrote: > > > > On 2012/05/17 21:44:47, Ami Fischman wrote: > > > > > This at least needs a TODO to support mid-stream frame size changes (and > > > test > > > > > that failure is graceful, not resulting in ASAN violations & security > > bugs). > > > > > > > > I'm not sure if this config info is related to mid-stream frame size > > changes. > > > > > > This config info includes width & height, and the mid-stream frame size > > changes > > > I'm talking about are changes in width & height. How can they be unrelated? > > > > So taking a step back. > > > > All the Mac video API client code that I've seen do this: > > VDACreate(extra_data) > > while (!end of data) { > > VDADecode(); > > } > > VDADestroy(); > > > > So if there's a frame size change in the middle of the stream I assume > > VDADecode() will either handle it or not handle it. Either way it doesn't seem > > like there's much I should be doing. > > Presumably you need to request different textures, no? > > > If you can point me at a sample video > > There's a mid-stream frame-size-changing .webm checked into > third_party/WebKit/LayoutTests/platform/chromium/media/resources/frame_size_change.webm > but that's webm. You'll have to transcode it to h264 to test. Sounds good. I've filed bug 130352 to track this. Will work on it ASAP. http://codereview.chromium.org/10388108/diff/25001/content/common/gpu/media/m... content/common/gpu/media/mac_video_decode_accelerator.mm:161: if (frame_buffer_size > max_frame_buffer_size) { On 2012/05/30 00:29:58, Ami Fischman wrote: > On 2012/05/29 03:45:01, sail wrote: > > On 2012/05/24 21:30:30, Ami Fischman wrote: > > > FWIW, writing this as > > > if (frame_buffer_size >> (nalu_len_field_size_ * 8)) > > > might be clearer. > > > > > > (if you keep m_f_b_s, make it const) > > > > Unfortunately that overflows when nalu_len_field_size_ is 4 and chrome is > built > > as a 32 bit app. > > WDYM "overflows"? Sorry, not sure what the correct term is. If I do: uint32_t a = 1024; uint32_t b = a >> 32; Then GCC gives the the following warning: warning: right shift count >= width of type And the value of b is 1024, not 0. http://codereview.chromium.org/10388108/diff/25001/content/common/gpu/media/m... content/common/gpu/media/mac_video_decode_accelerator.mm:287: client_->NotifyFlushDone(); On 2012/05/30 00:29:58, Ami Fischman wrote: > On 2012/05/29 03:45:01, sail wrote: > > On 2012/05/24 21:30:30, Ami Fischman wrote: > > > On 2012/05/23 23:02:29, sail wrote: > > > > On 2012/05/17 21:44:47, Ami Fischman wrote: > > > > > assert decoded_images_ is empty? > > > > > > > > Hm... it's possible that there are no available images so the decoded > images > > > > haven't been sent yet. (DXVAVideoDecodeAccelerator has the same issue). > > > In that case in what sense is the flush "done"??? > > So it looks like there are a few solutions: > > #1 wait for the decoded images to be sent to the client (as new pictures > > become available). > > This is the only option that's compatible with the API as described in > video_decode_accelerator.h. > > > #2 give up and empty the decoded images queue > > That's a partial-Reset(), not a Flush(). > > > #3 do nothing an send these images even after NotifyFlushDone is called. > > This is terrible ;) > A correct client would be within its rights to CHECK-fail. > > > Also, it looks like I'd have to implement this for all three decoders. > > Are you saying you think OVDA has this bug? Can you repro it in a unittest? > (it shouldn't) > > > If you want me to do #1 can I do it in a separate CL? > > As long as it gets done in the short term, sure. > Sounds good. I've filed bug 130357 to track this work. I'll also add a unit test for this. http://codereview.chromium.org/10388108/diff/32002/content/common/gpu/media/m... File content/common/gpu/media/mac_video_decode_accelerator.mm (right): http://codereview.chromium.org/10388108/diff/32002/content/common/gpu/media/m... content/common/gpu/media/mac_video_decode_accelerator.mm:175: "Bitstream buffer is too large.", INVALID_ARGUMENT,); On 2012/05/30 00:29:58, Ami Fischman wrote: > double space Done.
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sail@chromium.org/10388108/38001
Presubmit check for 10388108-38001 failed and returned exit status 1.
Running presubmit commit checks ...
** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
content
base/mac
ui/gfx
Presubmit checks took 5.1s to calculate.
thakis: base/mac OWNERS review sky: ui/gfx OWNERS review brettw: content/ OWNERS review
https://chromiumcodereview.appspot.com/10388108/diff/38001/base/mac/scoped_cf... File base/mac/scoped_cftyperef.h (right): https://chromiumcodereview.appspot.com/10388108/diff/38001/base/mac/scoped_cf... base/mac/scoped_cftyperef.h:41: explicit ScopedCFTypeRef(CFT object = NULL, Wouldn't it be nicer to have a global template function that basically does template<class T> T TypedCFRetain(T t) { return static_cast<T>(CFRetain(t)); } ? Then you could just write `ScopedCFTypeRef(TypedCFRetain(myobj))` and wouldn't need all the changes to this file.
https://chromiumcodereview.appspot.com/10388108/diff/38001/base/mac/scoped_cf... File base/mac/scoped_cftyperef.h (right): https://chromiumcodereview.appspot.com/10388108/diff/38001/base/mac/scoped_cf... base/mac/scoped_cftyperef.h:41: explicit ScopedCFTypeRef(CFT object = NULL, On 2012/05/30 23:33:39, Nico wrote: > Wouldn't it be nicer to have a global template function that basically does > > template<class T> > T TypedCFRetain(T t) { > return static_cast<T>(CFRetain(t)); > } > > ? Then you could just write `ScopedCFTypeRef(TypedCFRetain(myobj))` and wouldn't > need all the changes to this file. But ScopedCFTypeRef didn't have copy and assign which I needed. Basically, what I'm trying to do is store CVImageBufferRef in a STL container and not worry about memory management. It seemed like the simplest thing to do was support assignment operator and copy constructor.
On 2012/05/30 23:37:54, sail wrote: > https://chromiumcodereview.appspot.com/10388108/diff/38001/base/mac/scoped_cf... > File base/mac/scoped_cftyperef.h (right): > > https://chromiumcodereview.appspot.com/10388108/diff/38001/base/mac/scoped_cf... > base/mac/scoped_cftyperef.h:41: explicit ScopedCFTypeRef(CFT object = NULL, > On 2012/05/30 23:33:39, Nico wrote: > > Wouldn't it be nicer to have a global template function that basically does > > > > template<class T> > > T TypedCFRetain(T t) { > > return static_cast<T>(CFRetain(t)); > > } > > > > ? Then you could just write `ScopedCFTypeRef(TypedCFRetain(myobj))` and > wouldn't > > need all the changes to this file. > > But ScopedCFTypeRef didn't have copy and assign which I needed. > > Basically, what I'm trying to do is store CVImageBufferRef in a STL container > and not worry about memory management. > > It seemed like the simplest thing to do was support assignment operator and copy > constructor. I see. I'm ont sure if those operators are currently disallowed for a reason – can you ask whoever created this file for a review? (for normal scoped_ptr<>s, the reason is that there's no good implementation for the operators (that's why there's scoped_array). Maybe someone just copied from scoped_ptr<> as cf stuff is refcounted anyway; or maybe there's a reason for that.) -- It's usually a good idea to pull infrastructure changes like that into separate CLs (with a CL description that says "I want to use this in a container). Since it's a small CL, it will usually land long before your main CL anyway.
On 2012/05/30 23:41:25, Nico wrote: > On 2012/05/30 23:37:54, sail wrote: > > > https://chromiumcodereview.appspot.com/10388108/diff/38001/base/mac/scoped_cf... > > File base/mac/scoped_cftyperef.h (right): > > > > > https://chromiumcodereview.appspot.com/10388108/diff/38001/base/mac/scoped_cf... > > base/mac/scoped_cftyperef.h:41: explicit ScopedCFTypeRef(CFT object = NULL, > > On 2012/05/30 23:33:39, Nico wrote: > > > Wouldn't it be nicer to have a global template function that basically does > > > > > > template<class T> > > > T TypedCFRetain(T t) { > > > return static_cast<T>(CFRetain(t)); > > > } > > > > > > ? Then you could just write `ScopedCFTypeRef(TypedCFRetain(myobj))` and > > wouldn't > > > need all the changes to this file. > > > > But ScopedCFTypeRef didn't have copy and assign which I needed. > > > > Basically, what I'm trying to do is store CVImageBufferRef in a STL container > > and not worry about memory management. > > > > It seemed like the simplest thing to do was support assignment operator and > copy > > constructor. > > I see. I'm ont sure if those operators are currently disallowed for a reason – > can you ask whoever created this file for a review? (for normal scoped_ptr<>s, > the reason is that there's no good implementation for the operators (that's why > there's scoped_array). Maybe someone just copied from scoped_ptr<> as cf stuff > is refcounted anyway; or maybe there's a reason for that.) My changes are actually just a copy of the assignment operator and copy constructor that AVI added to scoped_nsprotocol. Adding avi and mark to the review (mark since he seems to have edited this file the most). > -- > > It's usually a good idea to pull infrastructure changes like that into separate > CLs (with a CL description that says "I want to use this in a container). Since > it's a small CL, it will usually land long before your main CL anyway. Will do next time.
mark, avi: could you take a look at my changes to base/mac. Thanks!
LGTM
I'm OK with it; Mark is the one to make happy. https://chromiumcodereview.appspot.com/10388108/diff/38001/base/mac/scoped_cf... File base/mac/scoped_cftyperef.h (right): https://chromiumcodereview.appspot.com/10388108/diff/38001/base/mac/scoped_cf... base/mac/scoped_cftyperef.h:31: // call CFRetain(). This behavior is parametrized by the |OwnershipPolicy| enum. parameterized
https://chromiumcodereview.appspot.com/10388108/diff/38001/base/mac/scoped_cf... File base/mac/scoped_cftyperef.h (right): https://chromiumcodereview.appspot.com/10388108/diff/38001/base/mac/scoped_cf... base/mac/scoped_cftyperef.h:17: enum OwnershipPolicy { Now we have base::mac::OwnershipPolicy here and scoped_policy::OwnershipPolicy from base/memory/scoped_nsobject.h. That kind of sucks. Can you come up with a good way to unify the two?
https://chromiumcodereview.appspot.com/10388108/diff/38001/base/mac/scoped_cf... File base/mac/scoped_cftyperef.h (right): https://chromiumcodereview.appspot.com/10388108/diff/38001/base/mac/scoped_cf... base/mac/scoped_cftyperef.h:17: enum OwnershipPolicy { On 2012/05/31 13:15:53, Mark Mentovai wrote: > Now we have base::mac::OwnershipPolicy here and scoped_policy::OwnershipPolicy > from base/memory/scoped_nsobject.h. That kind of sucks. Can you come up with a > good way to unify the two? Done. Moved to base/memory/scoped_policy.h https://chromiumcodereview.appspot.com/10388108/diff/38001/base/mac/scoped_cf... base/mac/scoped_cftyperef.h:31: // call CFRetain(). This behavior is parametrized by the |OwnershipPolicy| enum. On 2012/05/31 04:09:40, Avi wrote: > parameterized Done.
LGTM otherwise for base OWNERS approval. I only reviewed the files in base. https://chromiumcodereview.appspot.com/10388108/diff/34004/base/memory/scoped... File base/memory/scoped_policy.h (right): https://chromiumcodereview.appspot.com/10388108/diff/34004/base/memory/scoped... base/memory/scoped_policy.h:10: Eliminate this blank line. https://chromiumcodereview.appspot.com/10388108/diff/34004/base/memory/scoped... base/memory/scoped_policy.h:18: // The scoped object will retain the the object and any initial ownership is Blank line before. https://chromiumcodereview.appspot.com/10388108/diff/34004/base/memory/scoped... base/memory/scoped_policy.h:24: Eliminate this blank line too.
https://chromiumcodereview.appspot.com/10388108/diff/34004/base/memory/scoped... File base/memory/scoped_policy.h (right): https://chromiumcodereview.appspot.com/10388108/diff/34004/base/memory/scoped... base/memory/scoped_policy.h:10: On 2012/06/01 03:10:32, Mark Mentovai wrote: > Eliminate this blank line. Done. https://chromiumcodereview.appspot.com/10388108/diff/34004/base/memory/scoped... base/memory/scoped_policy.h:18: // The scoped object will retain the the object and any initial ownership is On 2012/06/01 03:10:32, Mark Mentovai wrote: > Blank line before. Done. https://chromiumcodereview.appspot.com/10388108/diff/34004/base/memory/scoped... base/memory/scoped_policy.h:24: On 2012/06/01 03:10:32, Mark Mentovai wrote: > Eliminate this blank line too. Done.
LGTM for base OWNERS approval.
content/*.gypi LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sail@chromium.org/10388108/32020
Try job failure for 10388108-32020 (retry) on mac_rel for step "compile" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sail@chromium.org/10388108/29008
Try job failure for 10388108-29008 (retry) on win for step "compile" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sail@chromium.org/10388108/40001
Try job failure for 10388108-40001 (retry) on win_rel for step "compile" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sail@chromium.org/10388108/40001
Change committed as 140153
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sail@chromium.org/10388108/56002
Change committed as 140187 |
