Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10899021:
Add CDM video decoder. (Closed)
|
Created:
8 years, 3 months ago by Tom Finegan Modified:
8 years, 2 months ago Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd CDM video decoder.
Add FFmpeg CDM video decoder, and fix bugs with passing emtpy resources through DecryptAndDecode, DeliverFrame, and Deliversamples.
TBR=brettw,viettrungluu
BUG=141780
TEST=*ExternalClearKey* browser tests pass
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=163501
Patch Set 1 #
Total comments: 46
Patch Set 2 : Another checkpoint. Still does not compile. #Patch Set 3 : Compiles, does nothing. #
Total comments: 10
Patch Set 4 : Rebased, and compiling again. Some comments addressed. #Patch Set 5 : Add VideoFrame class, nearing the point where a frame could be decoded... #Patch Set 6 : Needs to be hooked up in CdmWrapper #Patch Set 7 : Forward declare FFmpeg structs. #Patch Set 8 : Testing hackery... #Patch Set 9 : FFmpegVideoDecoder::Initialize now works. #Patch Set 10 : Could possibly work... #Patch Set 11 : Remove include from wrapper, fix a comment. #Patch Set 12 : Rebased. #Patch Set 13 : Compiles again. #Patch Set 14 : Rebased on PPAPI video decoder init CL. #Patch Set 15 : Maybe sorta could work... #
Total comments: 45
Patch Set 16 : Rebased (because WFH, sorry!). Fix VideoFrame and address comments. #Patch Set 17 : Renamings done. #Patch Set 18 : Allocate PP_DCHECK, rename video buffer callbacks, stop leaking video frames when FFmpeg releases t… #
Total comments: 53
Patch Set 19 : Comments addressed, and added some DVLOGs in the decoder wrapper. #Patch Set 20 : Address more comments. #
Total comments: 8
Patch Set 21 : Address more comments. #
Total comments: 40
Patch Set 22 : Invalidate source and copy missing values in TransferVideoFrame. #Patch Set 23 : Address more comments. #
Total comments: 5
Patch Set 24 : I think I got them all this time... #Patch Set 25 : Rebased #Patch Set 26 : Remove use of FFmpeg's get_buffer and release_buffer. #Patch Set 27 : Fix compile error, and handle cdm::kNeedMoreData in CdmWrapper::DeliverFrame(). #Patch Set 28 : Removed FFmpegCdmVideoFrame and added copying of video frames in FFmpegCdmVideoDecoder. #Patch Set 29 : Fix compile error. #
Total comments: 11
Patch Set 30 : Checks for odd dimensions, and remove unused include. #Patch Set 31 : Fix ExternalClearKey browser tests. #Patch Set 32 : Fix link error on linux_clang. #Patch Set 33 : Fix tests on linux. #
Total comments: 6
Patch Set 34 : Fix disable seccomp sandbox flag usage. #Patch Set 35 : Fix and enable the fake video decoder. #Patch Set 36 : Fix resource leaks (thanks, brettw!). Enable real video decoder. #Patch Set 37 : Rebased. #Patch Set 38 : Add media library initialization. #
Total comments: 6
Patch Set 39 : Add AtExitManager member to ClearKeyCdm. #Patch Set 40 : #Patch Set 41 : Rebased. #
Total comments: 10
Patch Set 42 : Address comment and fix gyp file. #Patch Set 43 : ffmpeg decoder ifdefs and more gyp changes. #
Total comments: 15
Patch Set 44 : Comments addressed. #Messages
Total messages: 53 (0 generated)
Very early version of the FFmpeg decode wrapper. Do not invest significant time reviewing this-- it doesn't even compile. Taking a look at the .h files might not be a bad idea, and some advice as to what I should and should not dupe (as opposed to using media stuff directly) is welcome! This is based *heavily* on the current media stack code.
http://codereview.chromium.org/10899021/diff/1/webkit/media/crypto/decoders/v... File webkit/media/crypto/decoders/video_decoder.h (right): http://codereview.chromium.org/10899021/diff/1/webkit/media/crypto/decoders/v... webkit/media/crypto/decoders/video_decoder.h:22: } This should not be here-- copy/paste error that I caught while giving the CL a quick look. It will live in ffmpeg_util unless I should rework things to use media::VideoFrame.
Reviewed only the header files (plus one .cc file). CDM.h looks fine, but it'd be good to get someone with more experience in that area to take a look. http://codereview.chromium.org/10899021/diff/1/webkit/media/crypto/decoders/f... File webkit/media/crypto/decoders/ffmpeg_util.h (right): http://codereview.chromium.org/10899021/diff/1/webkit/media/crypto/decoders/f... webkit/media/crypto/decoders/ffmpeg_util.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. Do we expect a lot of decoders for the clear key CDM? If not, I think this should just go in ppapi. A separate directory for a few files used for only one thing seems overkill. http://codereview.chromium.org/10899021/diff/1/webkit/media/crypto/decoders/v... File webkit/media/crypto/decoders/video_decoder.cc (right): http://codereview.chromium.org/10899021/diff/1/webkit/media/crypto/decoders/v... webkit/media/crypto/decoders/video_decoder.cc:11: VideoDecoder::~VideoDecoder() {} There might be an exception for inlining methods if you only need a virtual destructor. http://codereview.chromium.org/10899021/diff/1/webkit/media/crypto/decoders/v... File webkit/media/crypto/decoders/video_decoder.h (right): http://codereview.chromium.org/10899021/diff/1/webkit/media/crypto/decoders/v... webkit/media/crypto/decoders/video_decoder.h:33: VideoDecoder(const VideoDecoder&); private: DISALLOW_IMPLICIT_CONSTRUCTORS( VideoDecoder); or DISALLOW_COPY_AND_ASSIGN(VideoDecoder); requires base/basictypes.h http://codereview.chromium.org/10899021/diff/1/webkit/media/crypto/ppapi/clea... File webkit/media/crypto/ppapi/clear_key_cdm.cc (right): http://codereview.chromium.org/10899021/diff/1/webkit/media/crypto/ppapi/clea... webkit/media/crypto/ppapi/clear_key_cdm.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. FYI, we need to init FFmpeg when the plugin is loaded. xhwang is looking into this. http://codereview.chromium.org/10899021/diff/1/webkit/media/crypto/ppapi/clea... File webkit/media/crypto/ppapi/clear_key_cdm.h (right): http://codereview.chromium.org/10899021/diff/1/webkit/media/crypto/ppapi/clea... webkit/media/crypto/ppapi/clear_key_cdm.h:110: scoped_ptr<FFmpegVideoDecoder> video_decoder_; Why do we have a base class if even this header file relies on the concrete class? http://codereview.chromium.org/10899021/diff/1/webkit/media/crypto/ppapi/cont... File webkit/media/crypto/ppapi/content_decryption_module.h (right): http://codereview.chromium.org/10899021/diff/1/webkit/media/crypto/ppapi/cont... webkit/media/crypto/ppapi/content_decryption_module.h:138: enum Format { Maybe ColorFormat or something like that. Unless "Format" is commonly understood in this context. http://codereview.chromium.org/10899021/diff/1/webkit/media/crypto/ppapi/cont... webkit/media/crypto/ppapi/content_decryption_module.h:139: INVALID = 0, // Invalid format value. Used for error reporting. Check the naming in style guide. Might need more context in the name. http://codereview.chromium.org/10899021/diff/1/webkit/media/crypto/ppapi/cont... webkit/media/crypto/ppapi/content_decryption_module.h:141: YV12, // 12bpp YVU planar 1x1 Y, 2x2 VU samples. Should probably align comments in this case. http://codereview.chromium.org/10899021/diff/1/webkit/media/crypto/ppapi/cont... webkit/media/crypto/ppapi/content_decryption_module.h:143: EMPTY, // An empty frame. Why is EMPTY in the middle of YVU items? Should it be after INVALID instead? http://codereview.chromium.org/10899021/diff/1/webkit/media/crypto/ppapi/cont... webkit/media/crypto/ppapi/content_decryption_module.h:167: Size data_size; frame_size. data doesn't seem right. http://codereview.chromium.org/10899021/diff/1/webkit/media/crypto/ppapi/cont... webkit/media/crypto/ppapi/content_decryption_module.h:182: kUnknownVideoCodec = 0, This name is inconsistent with the rest. http://codereview.chromium.org/10899021/diff/1/webkit/media/crypto/ppapi/cont... webkit/media/crypto/ppapi/content_decryption_module.h:183: kCodecH264, kName vs. NAME for Format. Pick one - see style guide. http://codereview.chromium.org/10899021/diff/1/webkit/media/crypto/ppapi/cont... webkit/media/crypto/ppapi/content_decryption_module.h:184: kCodecVC1, We're not going to support these 4, so remove them. http://codereview.chromium.org/10899021/diff/1/webkit/media/crypto/ppapi/cont... webkit/media/crypto/ppapi/content_decryption_module.h:193: H264PROFILE_MIN = 0, I wonder if Chrome supports all these. http://codereview.chromium.org/10899021/diff/1/webkit/media/crypto/ppapi/cont... webkit/media/crypto/ppapi/content_decryption_module.h:214: VideoFrame::Format format; Why do both the frame and config have a format and size? http://codereview.chromium.org/10899021/diff/1/webkit/media/crypto/ppapi/cont... webkit/media/crypto/ppapi/content_decryption_module.h:215: VideoFrame::Size coded_size; coded? encoded? compressed? http://codereview.chromium.org/10899021/diff/1/webkit/media/crypto/ppapi/cont... webkit/media/crypto/ppapi/content_decryption_module.h:216: uint8_t* extra_data; What is this? http://codereview.chromium.org/10899021/diff/1/webkit/media/crypto/ppapi/cont... webkit/media/crypto/ppapi/content_decryption_module.h:277: // Initializes the CDM video decoder with |video_decoder_config|. This To keep Decrypt* together, I would move this above Decrypt(). http://codereview.chromium.org/10899021/diff/1/webkit/media/crypto/ppapi/cont... webkit/media/crypto/ppapi/content_decryption_module.h:288: // only empty (|format| being EMPTY) |video_frame| can be produced. Do we need a Flush() if no longer care about the frames? Maybe not since the plugin is destroyed when loading.
As the patch set title says: this now compiles. Not that I expect it to do anything. I need allocation to move forward, so I'm finally switching tasks. http://codereview.chromium.org/10899021/diff/1/webkit/media/crypto/decoders/f... File webkit/media/crypto/decoders/ffmpeg_util.h (right): http://codereview.chromium.org/10899021/diff/1/webkit/media/crypto/decoders/f... webkit/media/crypto/decoders/ffmpeg_util.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2012/08/29 17:33:49, ddorwin wrote: > Do we expect a lot of decoders for the clear key CDM? If not, I think this > should just go in ppapi. A separate directory for a few files used for only one > thing seems overkill. I thought it might be handy to have a decoders subdir for ffmpeg_vorbis_decoder (if we go with that name), and whatever we end up w/as far as libvpx and libvorbis. It also seems nice to hide everything ffmpeg related in a subdir. I'll move things up a level in a later patch set if you prefer. The above seems like hand waving now that I reread it. :) http://codereview.chromium.org/10899021/diff/1/webkit/media/crypto/decoders/v... File webkit/media/crypto/decoders/video_decoder.cc (right): http://codereview.chromium.org/10899021/diff/1/webkit/media/crypto/decoders/v... webkit/media/crypto/decoders/video_decoder.cc:11: VideoDecoder::~VideoDecoder() {} On 2012/08/29 17:33:49, ddorwin wrote: > There might be an exception for inlining methods if you only need a virtual > destructor. Deleted this file/moved empty methods to video_decoder.h. I await the review beating. :) http://codereview.chromium.org/10899021/diff/1/webkit/media/crypto/decoders/v... File webkit/media/crypto/decoders/video_decoder.h (right): http://codereview.chromium.org/10899021/diff/1/webkit/media/crypto/decoders/v... webkit/media/crypto/decoders/video_decoder.h:22: } On 2012/08/29 01:37:46, tomf wrote: > This should not be here-- copy/paste error that I caught while giving the CL a > quick look. > > It will live in ffmpeg_util unless I should rework things to use > media::VideoFrame. Done. http://codereview.chromium.org/10899021/diff/1/webkit/media/crypto/decoders/v... webkit/media/crypto/decoders/video_decoder.h:33: VideoDecoder(const VideoDecoder&); On 2012/08/29 17:33:49, ddorwin wrote: > private: > DISALLOW_IMPLICIT_CONSTRUCTORS( VideoDecoder); or > DISALLOW_COPY_AND_ASSIGN(VideoDecoder); > > requires base/basictypes.h Done. http://codereview.chromium.org/10899021/diff/1/webkit/media/crypto/ppapi/clea... File webkit/media/crypto/ppapi/clear_key_cdm.cc (right): http://codereview.chromium.org/10899021/diff/1/webkit/media/crypto/ppapi/clea... webkit/media/crypto/ppapi/clear_key_cdm.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2012/08/29 17:33:49, ddorwin wrote: > FYI, we need to init FFmpeg when the plugin is loaded. xhwang is looking into > this. Ok-- I'll finish up my decoder init and shutdown code, and punt on the actually loading FFmpeg part. http://codereview.chromium.org/10899021/diff/1/webkit/media/crypto/ppapi/clea... File webkit/media/crypto/ppapi/clear_key_cdm.h (right): http://codereview.chromium.org/10899021/diff/1/webkit/media/crypto/ppapi/clea... webkit/media/crypto/ppapi/clear_key_cdm.h:110: scoped_ptr<FFmpegVideoDecoder> video_decoder_; On 2012/08/29 17:33:49, ddorwin wrote: > Why do we have a base class if even this header file relies on the concrete > class? base::scoped_ptr<T> seems to be working fine w/forward declaration, and with scoped_ptr<T*> clang tells me I'm assigning to FFmpegVideoDecoder** at the assignment site in clear_key_cdm.cc. http://codereview.chromium.org/10899021/diff/1/webkit/media/crypto/ppapi/cont... File webkit/media/crypto/ppapi/content_decryption_module.h (right): http://codereview.chromium.org/10899021/diff/1/webkit/media/crypto/ppapi/cont... webkit/media/crypto/ppapi/content_decryption_module.h:138: enum Format { On 2012/08/29 17:33:49, ddorwin wrote: > Maybe ColorFormat or something like that. Unless "Format" is commonly understood > in this context. From src/media, but went with ColorFormat because I prefer the more descriptive type name. http://codereview.chromium.org/10899021/diff/1/webkit/media/crypto/ppapi/cont... webkit/media/crypto/ppapi/content_decryption_module.h:139: INVALID = 0, // Invalid format value. Used for error reporting. On 2012/08/29 17:33:49, ddorwin wrote: > Check the naming in style guide. Might need more context in the name. From src/media. Fixed this, and discarded everything but I420 and YV12. http://codereview.chromium.org/10899021/diff/1/webkit/media/crypto/ppapi/cont... webkit/media/crypto/ppapi/content_decryption_module.h:141: YV12, // 12bpp YVU planar 1x1 Y, 2x2 VU samples. On 2012/08/29 17:33:49, ddorwin wrote: > Should probably align comments in this case. Done. http://codereview.chromium.org/10899021/diff/1/webkit/media/crypto/ppapi/cont... webkit/media/crypto/ppapi/content_decryption_module.h:143: EMPTY, // An empty frame. On 2012/08/29 17:33:49, ddorwin wrote: > Why is EMPTY in the middle of YVU items? Should it be after INVALID instead? From src/media. http://codereview.chromium.org/10899021/diff/1/webkit/media/crypto/ppapi/cont... webkit/media/crypto/ppapi/content_decryption_module.h:167: Size data_size; On 2012/08/29 17:33:49, ddorwin wrote: > frame_size. data doesn't seem right. From src/media, but I prefer frame_size. Done. http://codereview.chromium.org/10899021/diff/1/webkit/media/crypto/ppapi/cont... webkit/media/crypto/ppapi/content_decryption_module.h:182: kUnknownVideoCodec = 0, On 2012/08/29 17:33:49, ddorwin wrote: > This name is inconsistent with the rest. From src/media. Dropped everything but unknown, h264, and vp8. Fixed inconsistent naming, and set kCodecVp8 to 6 so that the value matches the one from media. http://codereview.chromium.org/10899021/diff/1/webkit/media/crypto/ppapi/cont... webkit/media/crypto/ppapi/content_decryption_module.h:183: kCodecH264, On 2012/08/29 17:33:49, ddorwin wrote: > kName vs. NAME for Format. Pick one - see style guide. The naming inconsistency is due to src/media ancestry. I don't think fixing it hurts anything, so: Done. http://codereview.chromium.org/10899021/diff/1/webkit/media/crypto/ppapi/cont... webkit/media/crypto/ppapi/content_decryption_module.h:184: kCodecVC1, On 2012/08/29 17:33:49, ddorwin wrote: > We're not going to support these 4, so remove them. Done. http://codereview.chromium.org/10899021/diff/1/webkit/media/crypto/ppapi/cont... webkit/media/crypto/ppapi/content_decryption_module.h:193: H264PROFILE_MIN = 0, On 2012/08/29 17:33:49, ddorwin wrote: > I wonder if Chrome supports all these. From src/media, and probably something we should look into as we probably need only a few of these. http://codereview.chromium.org/10899021/diff/1/webkit/media/crypto/ppapi/cont... webkit/media/crypto/ppapi/content_decryption_module.h:214: VideoFrame::Format format; On 2012/08/29 17:33:49, ddorwin wrote: > Why do both the frame and config have a format and size? From src/media. I think this is because we need a format and a size to init the FFmpeg decoder, and we need the same at decode time (but we won't be hanging onto the config). http://codereview.chromium.org/10899021/diff/1/webkit/media/crypto/ppapi/cont... webkit/media/crypto/ppapi/content_decryption_module.h:215: VideoFrame::Size coded_size; On 2012/08/29 17:33:49, ddorwin wrote: > coded? > encoded? compressed? Done, s/coded_size/frame_size/. http://codereview.chromium.org/10899021/diff/1/webkit/media/crypto/ppapi/cont... webkit/media/crypto/ppapi/content_decryption_module.h:216: uint8_t* extra_data; On 2012/08/29 17:33:49, ddorwin wrote: > What is this? From src/media. Comment from media/base/video_decoder_config.h: // Optional byte data required to initialize video decoders, such as H.264 // AAVC data. http://codereview.chromium.org/10899021/diff/1/webkit/media/crypto/ppapi/cont... webkit/media/crypto/ppapi/content_decryption_module.h:277: // Initializes the CDM video decoder with |video_decoder_config|. This On 2012/08/29 17:33:49, ddorwin wrote: > To keep Decrypt* together, I would move this above Decrypt(). Done. http://codereview.chromium.org/10899021/diff/1/webkit/media/crypto/ppapi/cont... webkit/media/crypto/ppapi/content_decryption_module.h:288: // only empty (|format| being EMPTY) |video_frame| can be produced. On 2012/08/29 17:33:49, ddorwin wrote: > Do we need a Flush() if no longer care about the frames? Maybe not since the > plugin is destroyed when loading. We might need it for seeking, added.
scherkus, Added you to this review because this builds on the DecryptAndDecode changes xhwang has in http://codereview.chromium.org/10900007/, and I think I've already addressed some of your comments on the same code. We're sharing CL data again. Sorry for asking you to look at the same code more than once!
On 2012/09/01 21:42:16, tomf wrote: > scherkus, > > Added you to this review because this builds on the DecryptAndDecode changes > xhwang has in http://codereview.chromium.org/10900007/, and I think I've already > addressed some of your comments on the same code. > > We're sharing CL data again. Sorry for asking you to look at the same code more > than once! Btw, this isn't ready for a full review-- sorry for the extra spam!
Reviewed the header files again. Need to reconcile with http://codereview.chromium.org/10900007/. http://codereview.chromium.org/10899021/diff/1/webkit/media/crypto/decoders/f... File webkit/media/crypto/decoders/ffmpeg_util.h (right): http://codereview.chromium.org/10899021/diff/1/webkit/media/crypto/decoders/f... webkit/media/crypto/decoders/ffmpeg_util.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2012/08/30 17:09:23, tomf wrote: > On 2012/08/29 17:33:49, ddorwin wrote: > > Do we expect a lot of decoders for the clear key CDM? If not, I think this > > should just go in ppapi. A separate directory for a few files used for only > one > > thing seems overkill. > > I thought it might be handy to have a decoders subdir for ffmpeg_vorbis_decoder > (if we go with that name), and whatever we end up w/as far as libvpx and > libvorbis. It also seems nice to hide everything ffmpeg related in a subdir. > > I'll move things up a level in a later patch set if you prefer. The above seems > like hand waving now that I reread it. :) The decoders are explicitly for the clear key PPAPI CDM and not crypto in general, so having a subdir in crypto/ does not make sense. If anything, maybe we should move clearkey to a subdir of ppapi/ We'll use FFmpeg for all codecs, so we should have just one more (audio) set of files. http://codereview.chromium.org/10899021/diff/1/webkit/media/crypto/ppapi/clea... File webkit/media/crypto/ppapi/clear_key_cdm.h (right): http://codereview.chromium.org/10899021/diff/1/webkit/media/crypto/ppapi/clea... webkit/media/crypto/ppapi/clear_key_cdm.h:110: scoped_ptr<FFmpegVideoDecoder> video_decoder_; On 2012/08/30 17:09:23, tomf wrote: > On 2012/08/29 17:33:49, ddorwin wrote: > > Why do we have a base class if even this header file relies on the concrete > > class? > > base::scoped_ptr<T> seems to be working fine w/forward declaration, and with > scoped_ptr<T*> clang tells me I'm assigning to FFmpegVideoDecoder** at the > assignment site in clear_key_cdm.cc. My point is that we never use the VideoDecoder class, so why does it exist as an interface? IOW, let's eliminate the base class. http://codereview.chromium.org/10899021/diff/1/webkit/media/crypto/ppapi/cont... File webkit/media/crypto/ppapi/content_decryption_module.h (right): http://codereview.chromium.org/10899021/diff/1/webkit/media/crypto/ppapi/cont... webkit/media/crypto/ppapi/content_decryption_module.h:214: VideoFrame::Format format; On 2012/08/30 17:09:23, tomf wrote: > On 2012/08/29 17:33:49, ddorwin wrote: > > Why do both the frame and config have a format and size? > > From src/media. > > I think this is because we need a format and a size to init the FFmpeg decoder, > and we need the same at decode time (but we won't be hanging onto the config). It seems this would only be used for error checking. Otherwise, why doesn't the CDM just hang onto the values? (A question for scherkus.) http://codereview.chromium.org/10899021/diff/1/webkit/media/crypto/ppapi/cont... webkit/media/crypto/ppapi/content_decryption_module.h:216: uint8_t* extra_data; On 2012/08/30 17:09:23, tomf wrote: > On 2012/08/29 17:33:49, ddorwin wrote: > > What is this? > > From src/media. > > Comment from media/base/video_decoder_config.h: > // Optional byte data required to initialize video decoders, such as H.264 > // AAVC data. Will we definitely need this? If so, we should probably have a similar comment. Alternatively, maybe we can add it later. http://codereview.chromium.org/10899021/diff/9001/webkit/media/crypto/decoder... File webkit/media/crypto/decoders/ffmpeg_util.h (right): http://codereview.chromium.org/10899021/diff/9001/webkit/media/crypto/decoder... webkit/media/crypto/decoders/ffmpeg_util.h:18: #include <libavcodec/avcodec.h> We don't need all of these here. http://codereview.chromium.org/10899021/diff/9001/webkit/media/crypto/decoder... webkit/media/crypto/decoders/ffmpeg_util.h:32: cdm::VideoFrame::ColorFormat PixelFormatToVideoFormat(PixelFormat pixel_format); Only FFmpegVideoDecoder::GetVideoBuffer() uses this. If no other files will use it, is there a reason not to move it there? http://codereview.chromium.org/10899021/diff/9001/webkit/media/crypto/decoder... File webkit/media/crypto/decoders/ffmpeg_video_decoder.h (right): http://codereview.chromium.org/10899021/diff/9001/webkit/media/crypto/decoder... webkit/media/crypto/decoders/ffmpeg_video_decoder.h:34: cdm::VideoFrame* CreateVideoFrame(cdm::VideoFrame::ColorFormat format, A FrameDescription object would be useful here too to group these two params. http://codereview.chromium.org/10899021/diff/9001/webkit/media/crypto/decoder... File webkit/media/crypto/decoders/video_decoder.h (right): http://codereview.chromium.org/10899021/diff/9001/webkit/media/crypto/decoder... webkit/media/crypto/decoders/video_decoder.h:22: private: http://codereview.chromium.org/10899021/diff/9001/webkit/media/crypto/ppapi/c... File webkit/media/crypto/ppapi/content_decryption_module.h (right): http://codereview.chromium.org/10899021/diff/9001/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/content_decryption_module.h:139: // Values are skipped in here to keep them synchronized with Is there a reason these need to be synchronized? Avoiding switch statements? Same question for other enums. http://codereview.chromium.org/10899021/diff/9001/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/content_decryption_module.h:210: VideoFrame::ColorFormat format; Odd that we have to use the other class's enum. Maybe FrameInfo/FrameDescription should be a separate class (unless we can eliminate these from one of the classes altogether). http://codereview.chromium.org/10899021/diff/9001/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/content_decryption_module.h:269: virtual Status FlushVideoDecoder() = 0; Should we just have one Flush() for all streams? I guess that could not be used if you wanted to change just one one decoder, but maybe the Initialize* methods should handle that internally.
Only addresses comments on CDM interface. http://codereview.chromium.org/10899021/diff/9001/webkit/media/crypto/decoder... File webkit/media/crypto/decoders/video_decoder.h (right): http://codereview.chromium.org/10899021/diff/9001/webkit/media/crypto/decoder... webkit/media/crypto/decoders/video_decoder.h:13: class VideoDecoder { Media code try to avoid defining interface classes if there's only one implementation. Other CDMs may not want to implement this interface if they want to protect the decrypted compressed frames. http://codereview.chromium.org/10899021/diff/9001/webkit/media/crypto/ppapi/c... File webkit/media/crypto/ppapi/content_decryption_module.h (right): http://codereview.chromium.org/10899021/diff/9001/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/content_decryption_module.h:210: VideoFrame::ColorFormat format; On 2012/09/02 21:39:08, ddorwin wrote: > Odd that we have to use the other class's enum. Maybe FrameInfo/FrameDescription > should be a separate class (unless we can eliminate these from one of the > classes altogether). Done in http://codereview.chromium.org/10900007/ http://codereview.chromium.org/10899021/diff/9001/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/content_decryption_module.h:269: virtual Status FlushVideoDecoder() = 0; On 2012/09/02 21:39:08, ddorwin wrote: > Should we just have one Flush() for all streams? I guess that could not be used > if you wanted to change just one one decoder, but maybe the Initialize* methods > should handle that internally. From media pipeline, the flush calls come from VideoRenderer and AudioRenderer separately. If these two calls come out-of-sync, then we could end up with: 1, audio_decoder.flush() // CDM calls flush() and flushes both video and audio decoder 2. video_decoder.Decode() // oops, error since the video decoder's state changed! 3, video_decoder.flush()... This may cause pipeline error.
Ignore comments in CDM.h. These are now addressed in http://codereview.chromium.org/10900007/. Address comments in the other file, then you can rebase after that one lands.
PTAL, I think any standing comments have been addressed. This is not finished yet, but it's close to the point where it might work. No big deal if you guys want to hold off on looking at this until the changes to DeliverFrame are ready. http://codereview.chromium.org/10899021/diff/1/webkit/media/crypto/ppapi/clea... File webkit/media/crypto/ppapi/clear_key_cdm.cc (right): http://codereview.chromium.org/10899021/diff/1/webkit/media/crypto/ppapi/clea... webkit/media/crypto/ppapi/clear_key_cdm.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2012/08/30 17:09:23, tomf wrote: > On 2012/08/29 17:33:49, ddorwin wrote: > > FYI, we need to init FFmpeg when the plugin is loaded. xhwang is looking into > > this. > > Ok-- I'll finish up my decoder init and shutdown code, and punt on the actually > loading FFmpeg part. > FFmpegVideoDecoder calls av_register_all(). Not sure if that's good or bad, but it's required for subsequent ffmpeg calls to work. http://codereview.chromium.org/10899021/diff/1/webkit/media/crypto/ppapi/clea... File webkit/media/crypto/ppapi/clear_key_cdm.h (right): http://codereview.chromium.org/10899021/diff/1/webkit/media/crypto/ppapi/clea... webkit/media/crypto/ppapi/clear_key_cdm.h:110: scoped_ptr<FFmpegVideoDecoder> video_decoder_; On 2012/09/02 21:39:07, ddorwin wrote: > On 2012/08/30 17:09:23, tomf wrote: > > On 2012/08/29 17:33:49, ddorwin wrote: > > > Why do we have a base class if even this header file relies on the concrete > > > class? > > > > base::scoped_ptr<T> seems to be working fine w/forward declaration, and with > > scoped_ptr<T*> clang tells me I'm assigning to FFmpegVideoDecoder** at the > > assignment site in clear_key_cdm.cc. > > My point is that we never use the VideoDecoder class, so why does it exist as an > interface? IOW, let's eliminate the base class. Done.
Let the review beat down commence... PTAL. :) Thanks!
The biggest issue is at ffmpeg_video_decoder.h:42. We need to figure out a solution there. I did not thoroughly review the last 4 files. http://codereview.chromium.org/10899021/diff/47001/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/cdm_wrapper.cc (right): http://codereview.chromium.org/10899021/diff/47001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/cdm_wrapper.cc:131: if (stream_type == PP_DECRYPTORSTREAMTYPE_AUDIO) Use switch. Depending on settings, can ensure you don't miss any, and you can DCHECK unexpected (will NOTREACHED work here?). http://codereview.chromium.org/10899021/diff/47001/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/clear_key_cdm.cc (right): http://codereview.chromium.org/10899021/diff/47001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/clear_key_cdm.cc:208: DVLOG(1) << "ClearKeyCdm::Decrypt()"; Filename is already included in the log message. http://codereview.chromium.org/10899021/diff/47001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/clear_key_cdm.cc:270: Need to handle kDecryptError. Then DCHECK(kSuccess). http://codereview.chromium.org/10899021/diff/47001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/clear_key_cdm.cc:272: int data_size = buffer->GetDataSize(); why do we need this? http://codereview.chromium.org/10899021/diff/47001/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/ffmpeg_video_decoder.cc (right): http://codereview.chromium.org/10899021/diff/47001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_video_decoder.cc:72: DCHECK(config); NOTIMPLEMENTED()? http://codereview.chromium.org/10899021/diff/47001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_video_decoder.cc:88: } // anonymous name space // namespace http://codereview.chromium.org/10899021/diff/47001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_video_decoder.cc:177: void CopyCdmVideoFrame(cdm::VideoFrame* source, cdm::VideoFrame* target) { More of a Move. Maybe we should clear source as we go. http://codereview.chromium.org/10899021/diff/47001/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/ffmpeg_video_decoder.h (right): http://codereview.chromium.org/10899021/diff/47001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_video_decoder.h:18: static const uint32_t kBufferAlignment = 32; media/base has constants for these. http://codereview.chromium.org/10899021/diff/47001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_video_decoder.h:24: class FFmpegVideoDecoder { I worry about having identically named classes and files. It may lead to confusion when searching for code, etc. http://codereview.chromium.org/10899021/diff/47001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_video_decoder.h:32: remove line http://codereview.chromium.org/10899021/diff/47001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_video_decoder.h:34: //|decompressed_frame| when an output frame is available. Returns true when param name does not match http://codereview.chromium.org/10899021/diff/47001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_video_decoder.h:36: // Note: This method can return true without providing an output frame. This with a null |decoded_frame|. http://codereview.chromium.org/10899021/diff/47001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_video_decoder.h:40: int32_t frame_length, compressed_frame_size? http://codereview.chromium.org/10899021/diff/47001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_video_decoder.h:42: cdm::VideoFrame* decoded_frame); I think this CL shows we have a fundamental problem. VideoFrames are supposed to always be allocated by the wrapper, BUT FFmpeg can provide data at any point. As a result, we have an internal VideoFrame structure that stores a cdm::VideoFrame (not allocated right now) that must be copied to this parameter. Part of the problem is that FFmpeg is apparently multithreaded and we assumed CDMs would not (need to) be. I guess we should support it, but that may mean interface changes (unless we can use FFmpeg single-threaded). I think a better solution might be to have Allocator return VideoFrames and make this a double pointer. The only issue is that you'll need to keep track of all the allocated frames, which you probably need to do in the current design. http://codereview.chromium.org/10899021/diff/47001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_video_decoder.h:47: int GetVideoBuffer(AVCodecContext *codec_context, AVFrame* frame); *<space> http://codereview.chromium.org/10899021/diff/47001/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/video_frame.cc (right): http://codereview.chromium.org/10899021/diff/47001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/video_frame.cc:12: namespace { This should be inside the other namespace. media/ tends to use static instead of anonymous namespaces. :( http://codereview.chromium.org/10899021/diff/47001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/video_frame.cc:34: NOTREACHED() << "Unsupported video frame format: " << format; Put these lines in default. http://codereview.chromium.org/10899021/diff/47001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/video_frame.cc:65: frame_(NULL) { Where is frame_ set? It must be requested from the wrapper, right? http://codereview.chromium.org/10899021/diff/47001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/video_frame.cc:70: frame_->frame_buffer()->Destroy(); Does cdm::~VideoFrameImpl() not do this? http://codereview.chromium.org/10899021/diff/47001/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/video_frame.h (right): http://codereview.chromium.org/10899021/diff/47001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/video_frame.h:14: class VideoFrame { What is the purpose of this class? http://codereview.chromium.org/10899021/diff/47001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/video_frame.h:22: int64_t timestamp); timestamp is not used. http://codereview.chromium.org/10899021/diff/47001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/video_frame.h:45: cdm::VideoFrame* frame_; Who owns this?
Now with a webkit_media::VideoFrame that actually has the potential of working. I wasn't sure about all comments. PTAL. Thanks, and sorry about the rebase... moved to home PC. http://codereview.chromium.org/10899021/diff/47001/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/cdm_wrapper.cc (right): http://codereview.chromium.org/10899021/diff/47001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/cdm_wrapper.cc:131: if (stream_type == PP_DECRYPTORSTREAMTYPE_AUDIO) On 2012/10/13 03:54:42, ddorwin wrote: > Use switch. Depending on settings, can ensure you don't miss any, and you can > DCHECK unexpected (will NOTREACHED work here?). Done. http://codereview.chromium.org/10899021/diff/47001/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/clear_key_cdm.cc (right): http://codereview.chromium.org/10899021/diff/47001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/clear_key_cdm.cc:208: DVLOG(1) << "ClearKeyCdm::Decrypt()"; On 2012/10/13 03:54:42, ddorwin wrote: > Filename is already included in the log message. Done. http://codereview.chromium.org/10899021/diff/47001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/clear_key_cdm.cc:270: On 2012/10/13 03:54:42, ddorwin wrote: > Need to handle kDecryptError. Then DCHECK(kSuccess). Done. http://codereview.chromium.org/10899021/diff/47001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/clear_key_cdm.cc:272: int data_size = buffer->GetDataSize(); On 2012/10/13 03:54:42, ddorwin wrote: > why do we need this? Removed. http://codereview.chromium.org/10899021/diff/47001/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/ffmpeg_video_decoder.cc (right): http://codereview.chromium.org/10899021/diff/47001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_video_decoder.cc:72: DCHECK(config); On 2012/10/13 03:54:42, ddorwin wrote: > NOTIMPLEMENTED()? Forgot to remove this... something I thought I needed before we know how init would work. Removed. http://codereview.chromium.org/10899021/diff/47001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_video_decoder.cc:88: } // anonymous name space On 2012/10/13 03:54:42, ddorwin wrote: > // namespace Done. http://codereview.chromium.org/10899021/diff/47001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_video_decoder.cc:177: void CopyCdmVideoFrame(cdm::VideoFrame* source, cdm::VideoFrame* target) { On 2012/10/13 03:54:42, ddorwin wrote: > More of a Move. Maybe we should clear source as we go. Renamed to MoveCdmVideoFrame(), changed arg 1 to webkit_media::VideoFrame*, and added call to ReleaseCdmVideoFrame() at the end. http://codereview.chromium.org/10899021/diff/47001/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/ffmpeg_video_decoder.h (right): http://codereview.chromium.org/10899021/diff/47001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_video_decoder.h:18: static const uint32_t kBufferAlignment = 32; On 2012/10/13 03:54:42, ddorwin wrote: > media/base has constants for these. The only ones I can find are in .cc files. Should I move these to the file where they are used? http://codereview.chromium.org/10899021/diff/47001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_video_decoder.h:24: class FFmpegVideoDecoder { On 2012/10/13 03:54:42, ddorwin wrote: > I worry about having identically named classes and files. It may lead to > confusion when searching for code, etc. Yeah, I use cs.chromium.org a lot... I can see that being a serious annoyance. How about: Rename classes to CdmFfmpegVideoDecoder and CdmVideoFrame Rename files to cdm_ffmpeg_video_decoder and cdm_video_frame Not sure if those are unique enough, and CdmVideoFrame might be confusing because of cdm::VideoFrame. http://codereview.chromium.org/10899021/diff/47001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_video_decoder.h:32: On 2012/10/13 03:54:42, ddorwin wrote: > remove line Done. http://codereview.chromium.org/10899021/diff/47001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_video_decoder.h:34: //|decompressed_frame| when an output frame is available. Returns true when On 2012/10/13 03:54:42, ddorwin wrote: > param name does not match Done. http://codereview.chromium.org/10899021/diff/47001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_video_decoder.h:36: // Note: This method can return true without providing an output frame. This On 2012/10/13 03:54:42, ddorwin wrote: > with a null |decoded_frame|. Done. http://codereview.chromium.org/10899021/diff/47001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_video_decoder.h:40: int32_t frame_length, On 2012/10/13 03:54:42, ddorwin wrote: > compressed_frame_size? Done. http://codereview.chromium.org/10899021/diff/47001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_video_decoder.h:42: cdm::VideoFrame* decoded_frame); On 2012/10/13 03:54:42, ddorwin wrote: > I think this CL shows we have a fundamental problem. VideoFrames are supposed to > always be allocated by the wrapper, BUT FFmpeg can provide data at any point. As > a result, we have an internal VideoFrame structure that stores a cdm::VideoFrame > (not allocated right now) that must be copied to this parameter. > > Part of the problem is that FFmpeg is apparently multithreaded and we assumed > CDMs would not (need to) be. I guess we should support it, but that may mean > interface changes (unless we can use FFmpeg single-threaded). > > I think a better solution might be to have Allocator return VideoFrames and make > this a double pointer. The only issue is that you'll need to keep track of all > the allocated frames, which you probably need to do in the current design. The video frame memory is allocated using the allocator in GetVideoBuffer() (via VideoFrame::Create()). We may need a lock in allocator (I think using the ppapi lock util will break in process, though), or we can pass threads = 1 to ffmpeg. That's going to hurt decode speed, but maybe it doesn't matter since this is mainly for testing. http://codereview.chromium.org/10899021/diff/47001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_video_decoder.h:47: int GetVideoBuffer(AVCodecContext *codec_context, AVFrame* frame); On 2012/10/13 03:54:42, ddorwin wrote: > *<space> Done. http://codereview.chromium.org/10899021/diff/47001/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/video_frame.cc (right): http://codereview.chromium.org/10899021/diff/47001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/video_frame.cc:12: namespace { On 2012/10/13 03:54:42, ddorwin wrote: > This should be inside the other namespace. > media/ tends to use static instead of anonymous namespaces. :( Done. http://codereview.chromium.org/10899021/diff/47001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/video_frame.cc:34: NOTREACHED() << "Unsupported video frame format: " << format; On 2012/10/13 03:54:42, ddorwin wrote: > Put these lines in default. Done. http://codereview.chromium.org/10899021/diff/47001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/video_frame.cc:65: frame_(NULL) { On 2012/10/13 03:54:42, ddorwin wrote: > Where is frame_ set? It must be requested from the wrapper, right? I removed the cdm::VideoFrame abuse. What I had would not have worked. There's no way to create VideoFrameImpl here, and I don't really need it. VideoFrame now has members that hold the data that would've been stored in cdm::VideoFrame. http://codereview.chromium.org/10899021/diff/47001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/video_frame.cc:70: frame_->frame_buffer()->Destroy(); On 2012/10/13 03:54:42, ddorwin wrote: > Does cdm::~VideoFrameImpl() not do this? This class now creates a cdm::Buffer that it owns until ReleaseCdmBuffer is called. http://codereview.chromium.org/10899021/diff/47001/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/video_frame.h (right): http://codereview.chromium.org/10899021/diff/47001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/video_frame.h:14: class VideoFrame { On 2012/10/13 03:54:42, ddorwin wrote: > What is the purpose of this class? If you meant, "where's the comment", done. Otherwise, do you mean get rid of this class and source file? I could move this stuff into ffmpeg_video_decoder, but it seemed better to have it on its own. Maybe it makes more sense now that I removed cdm::VideoFrame. http://codereview.chromium.org/10899021/diff/47001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/video_frame.h:22: int64_t timestamp); On 2012/10/13 03:54:42, ddorwin wrote: > timestamp is not used. Removed. http://codereview.chromium.org/10899021/diff/47001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/video_frame.h:45: cdm::VideoFrame* frame_; On 2012/10/13 03:54:42, ddorwin wrote: > Who owns this? Added comment.
http://codereview.chromium.org/10899021/diff/47001/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/ffmpeg_video_decoder.h (right): http://codereview.chromium.org/10899021/diff/47001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_video_decoder.h:42: cdm::VideoFrame* decoded_frame); On 2012/10/13 23:47:26, Tom Finegan wrote: > On 2012/10/13 03:54:42, ddorwin wrote: > > I think this CL shows we have a fundamental problem. VideoFrames are supposed > to > > always be allocated by the wrapper, BUT FFmpeg can provide data at any point. > As > > a result, we have an internal VideoFrame structure that stores a > cdm::VideoFrame > > (not allocated right now) that must be copied to this parameter. > > > > Part of the problem is that FFmpeg is apparently multithreaded and we assumed > > CDMs would not (need to) be. I guess we should support it, but that may mean > > interface changes (unless we can use FFmpeg single-threaded). > > > > I think a better solution might be to have Allocator return VideoFrames and > make > > this a double pointer. The only issue is that you'll need to keep track of all > > the allocated frames, which you probably need to do in the current design. > > The video frame memory is allocated using the allocator in GetVideoBuffer() (via > VideoFrame::Create()). We may need a lock in allocator (I think using the ppapi > lock util will break in process, though), or we can pass threads = 1 to ffmpeg. > That's going to hurt decode speed, but maybe it doesn't matter since this is > mainly for testing. Actually, thinking about this some more, maybe this isn't going to work with the calls to the allocator coming from a different thread. I'm not sure if that's going to work. Maybe we have to run ffmpeg single threaded.
I'm not able to run the decrypt demo right now... trying to sort that out, but while I figure that out I think this is ready for review. I'm nervous about the PP_DCHECK in PpbBufferAllocator::Allocate breaking things, but it's probably OK. I just wanted to see it working in and out of process before pushing it up with this CL. I'll get a build working tonight and confirm things work before this lands. (assuming that it gets some lgtm's tonight, anyway). Anyway, PTAL. Thanks!
On 2012/10/16 01:36:11, Tom Finegan wrote: > I'm not able to run the decrypt demo right now... trying to sort that out, but > while I figure that out I think this is ready for review. > > I'm nervous about the PP_DCHECK in PpbBufferAllocator::Allocate breaking things, > but it's probably OK. I just wanted to see it working in and out of process > before pushing it up with this CL. I'll get a build working tonight and confirm > things work before this lands. (assuming that it gets some lgtm's tonight, > anyway). > Not sure what's up with the build on my linux box. I'll clobber it tomorrow. Finally tested this on windows, and DCHECKing IsMainThread() in the allocator works just fine in and out of process. > Anyway, PTAL. Thanks!
Initial review of the .h files with a quick pass over the new .cc files. http://codereview.chromium.org/10899021/diff/45009/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/clear_key_cdm.cc (right): http://codereview.chromium.org/10899021/diff/45009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/clear_key_cdm.cc:239: video_decoder_.reset(new webkit_media::FFmpegCdmVideoDecoder(allocator_)); What is the intended behavior if this is called a second time? Should it fail, succeed silently, or replace the decoder (as is the case now)? http://codereview.chromium.org/10899021/diff/45009/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.cc (right): http://codereview.chromium.org/10899021/diff/45009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.cc:180: void MoveVideoFrame(FFmpegCdmVideoFrame* source, cdm::VideoFrame* target) { This should be above with the other static/anonymous functions. This seems to make more sense as a method on FFmpegCdmVideoFrame, though. Then we don't need ReleaseCdmBuffer(). WDYT? http://codereview.chromium.org/10899021/diff/45009/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.h (right): http://codereview.chromium.org/10899021/diff/45009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.h:10: #include "base/memory/ref_counted.h" What is ref counted? http://codereview.chromium.org/10899021/diff/45009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.h:22: bool IsValidConfig(cdm::VideoFormat format, const cdm::Size& data_size); Should this all be in the cdm namespace? It's only used by the cdm. Also, it uses cdm types. This function is the most important to get right since it's just sitting in the webkit_media namespace. http://codereview.chromium.org/10899021/diff/45009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.h:33: // when an output frame is available. Returns true when when successful. "Returns whether the call was successful." Then you can drop the last sentence below. http://codereview.chromium.org/10899021/diff/45009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.h:45: int GetVideoBuffer(AVCodecContext* codec_context, AVFrame* frame); Can this be private? It's provided to FFmpeg as a pointer, right? http://codereview.chromium.org/10899021/diff/45009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.h:50: // FFmpeg structures owned by this object. Can we use scoped_ptr since these are owned? Or is that too much of base? http://codereview.chromium.org/10899021/diff/45009/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/ffmpeg_cdm_video_frame.h (right): http://codereview.chromium.org/10899021/diff/45009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_video_frame.h:19: // Returns a pointer to FFmpegCdmVideoFrame of the specified |format| and newline before the comment. http://codereview.chromium.org/10899021/diff/45009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_video_frame.h:28: int32_t plane_offset(cdm::VideoFrame::VideoPlane plane); const on both these accessors? http://codereview.chromium.org/10899021/diff/45009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_video_frame.h:51: cdm::Buffer* frame_buffer_; scoped_ptr?
Comments mostly addressed. Some comments seemed to warrant further discussion. Also added some DVLOGs for debugging this once it is hooked up... I'm pretty sure it's going to be at least slightly broken at first. :( PTAL! Thanks. http://codereview.chromium.org/10899021/diff/45009/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/clear_key_cdm.cc (right): http://codereview.chromium.org/10899021/diff/45009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/clear_key_cdm.cc:239: video_decoder_.reset(new webkit_media::FFmpegCdmVideoDecoder(allocator_)); On 2012/10/16 18:04:30, ddorwin wrote: > What is the intended behavior if this is called a second time? Should it fail, > succeed silently, or replace the decoder (as is the case now)? I think replacing the decoder is best, and not only because I'm being lazy! :) Succeeding silently would require inspecting the config (including a byte-by-byte compare of the extra data) to confirm it had not changed, and I think failing should only happen when the config is not supported. Sending DecoderInitializeDone with success equal to false just because the caller is "doing it wrong" seems like an artificial restriction when (as things are currently) we can support the re-initialization. WDYT? http://codereview.chromium.org/10899021/diff/45009/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.cc (right): http://codereview.chromium.org/10899021/diff/45009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.cc:180: void MoveVideoFrame(FFmpegCdmVideoFrame* source, cdm::VideoFrame* target) { On 2012/10/16 18:04:30, ddorwin wrote: > This should be above with the other static/anonymous functions. > > This seems to make more sense as a method on FFmpegCdmVideoFrame, though. Then > we don't need ReleaseCdmBuffer(). WDYT? Agreed and done. Not sure if MoveVideoFrame() is still the best name, though. http://codereview.chromium.org/10899021/diff/45009/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.h (right): http://codereview.chromium.org/10899021/diff/45009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.h:10: #include "base/memory/ref_counted.h" On 2012/10/16 18:04:30, ddorwin wrote: > What is ref counted? Removed, vestige of media/filters/ffmpeg_video_decoder.cc ancestry. http://codereview.chromium.org/10899021/diff/45009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.h:22: bool IsValidConfig(cdm::VideoFormat format, const cdm::Size& data_size); On 2012/10/16 18:04:30, ddorwin wrote: > Should this all be in the cdm namespace? It's only used by the cdm. Also, it > uses cdm types. This function is the most important to get right since it's just > sitting in the webkit_media namespace. This should not have been here. Made it a static member of FFmpegCdmVideoFrame since confirming that video frame creation is possible is the purpose of the config validation. http://codereview.chromium.org/10899021/diff/45009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.h:33: // when an output frame is available. Returns true when when successful. On 2012/10/16 18:04:30, ddorwin wrote: > "Returns whether the call was successful." > > Then you can drop the last sentence below. Done. http://codereview.chromium.org/10899021/diff/45009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.h:45: int GetVideoBuffer(AVCodecContext* codec_context, AVFrame* frame); On 2012/10/16 18:04:30, ddorwin wrote: > Can this be private? It's provided to FFmpeg as a pointer, right? This is what GetCdmVideoBufferImpl() calls, and that's why it was public. I've made GetCdmVideoBufferImpl() a friend, and made it non-static (required to friend it). This puts GetCdmVideoBufferImpl() in the webkit_media namespace, while ReleaseCdmBufferImpl() remains static. Declaring the get and release methods differently seems a little weird, but it should work. Should I keep it this way, or change it back? http://codereview.chromium.org/10899021/diff/45009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.h:50: // FFmpeg structures owned by this object. On 2012/10/16 18:04:30, ddorwin wrote: > Can we use scoped_ptr since these are owned? Or is that too much of base? Discussed offline. Not really too much base, more that we need wrapper classes for any FFmpeg objects that we want to put in scopers because we must pass FFmpeg objects to av_free() instead of using delete. http://codereview.chromium.org/10899021/diff/45009/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/ffmpeg_cdm_video_frame.h (right): http://codereview.chromium.org/10899021/diff/45009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_video_frame.h:19: // Returns a pointer to FFmpegCdmVideoFrame of the specified |format| and On 2012/10/16 18:04:30, ddorwin wrote: > newline before the comment. Done. http://codereview.chromium.org/10899021/diff/45009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_video_frame.h:28: int32_t plane_offset(cdm::VideoFrame::VideoPlane plane); On 2012/10/16 18:04:30, ddorwin wrote: > const on both these accessors? Done. http://codereview.chromium.org/10899021/diff/45009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_video_frame.h:51: cdm::Buffer* frame_buffer_; On 2012/10/16 18:04:30, ddorwin wrote: > scoped_ptr? Discussed offline. Same issue as w/the FFmpeg objects.
http://codereview.chromium.org/10899021/diff/45009/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/cdm_wrapper.cc (right): http://codereview.chromium.org/10899021/diff/45009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/cdm_wrapper.cc:621: void CdmWrapper::DeinitializeDecoder(PP_DecryptorStreamType decoder_type, s/decoder_type/stream_type ? http://codereview.chromium.org/10899021/diff/45009/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/clear_key_cdm.cc (right): http://codereview.chromium.org/10899021/diff/45009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/clear_key_cdm.cc:239: video_decoder_.reset(new webkit_media::FFmpegCdmVideoDecoder(allocator_)); On 2012/10/16 18:04:30, ddorwin wrote: > What is the intended behavior if this is called a second time? Should it fail, > succeed silently, or replace the decoder (as is the case now)? If the decoder is initialized, DeinitializeDecoder() should be called before it can be initialized again. We can add check here that the decoder is in an uninitialized state. Or we can defer this to the CDM to figure out. http://codereview.chromium.org/10899021/diff/45009/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.cc (right): http://codereview.chromium.org/10899021/diff/45009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.cc:98: static int GetCdmVideoBufferImpl(AVCodecContext* s, AVFrame* frame) { We are mixing anonymous namespace and static functions in this file. Stick to static since src/media uses it? http://codereview.chromium.org/10899021/diff/45009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.cc:164: // TODO(tomfinegan): Do we need to copy the extra data? Zeroing the members We don't. extra_data is only useful for initialization. It seems to me that we are not using config_ anywhere in this file. Remove? http://codereview.chromium.org/10899021/diff/45009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.cc:177: avcodec_flush_buffers(codec_context_); Do we want to have a flag or state to remember this? After avcoded_flush_buffers, we should only accept empty inputs. http://codereview.chromium.org/10899021/diff/45009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.cc:188: for (int i = 0; static_cast<VideoPlane>(i) < kMaxPlanes; ++i) { kMaxPlanes is an int. Why do we need the static_cast here? http://codereview.chromium.org/10899021/diff/45009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.cc:204: // Due to FFmpeg API changes we no longer have const read-only pointers. put this above line 207? http://codereview.chromium.org/10899021/diff/45009/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.h (right): http://codereview.chromium.org/10899021/diff/45009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.h:36: // false upon failure. During EOS, we'll call DecodeFrame with NULL input until there's no output available, in which case the return is true and the output is NULL. We should be able to distinguish this case with NoMoreData. http://codereview.chromium.org/10899021/diff/45009/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/ffmpeg_cdm_video_frame.cc (right): http://codereview.chromium.org/10899021/diff/45009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_video_frame.cc:79: return format_; accessors can be defined in header files http://codereview.chromium.org/10899021/diff/45009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_video_frame.cc:96: remove extra empty line http://codereview.chromium.org/10899021/diff/45009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_video_frame.cc:103: bool FFmpegCdmVideoFrame::AllocateYUV(cdm::Allocator* allocator) { Bikeshedding: Not sure if it should be YUV or Yuv :) http://codereview.chromium.org/10899021/diff/45009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_video_frame.cc:108: DVLOG(1) << "VideoFrame::AllocateYUV: width=" << size_.width s/VideoFame/FFmpegCdmVideoFrame http://codereview.chromium.org/10899021/diff/45009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_video_frame.cc:118: size_t uv_height = format_ == cdm::kYv12 ? y_height / 2 : y_height; The difference b/w YV12 and I420 is that YV12 is YVU but I420 is YUV. In case of I420, the uv_height should still be y_height / 2, right?
Some comments addressed, and some with follow up discussion. PTAL! Thanks. http://codereview.chromium.org/10899021/diff/45009/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/cdm_wrapper.cc (right): http://codereview.chromium.org/10899021/diff/45009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/cdm_wrapper.cc:621: void CdmWrapper::DeinitializeDecoder(PP_DecryptorStreamType decoder_type, On 2012/10/16 21:59:53, xhwang wrote: > s/decoder_type/stream_type ? I prefer decoder_type here. I think it fits better with DeinitializeDecoder... if/when we add a stream_id to these methods stream_type might be better. I also want the names to match the pepper API, and I used decoder_type throughout. Do you feel really strongly about this? http://codereview.chromium.org/10899021/diff/45009/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/clear_key_cdm.cc (right): http://codereview.chromium.org/10899021/diff/45009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/clear_key_cdm.cc:239: video_decoder_.reset(new webkit_media::FFmpegCdmVideoDecoder(allocator_)); On 2012/10/16 21:59:53, xhwang wrote: > On 2012/10/16 18:04:30, ddorwin wrote: > > What is the intended behavior if this is called a second time? Should it fail, > > succeed silently, or replace the decoder (as is the case now)? > > If the decoder is initialized, DeinitializeDecoder() should be called before it > can be initialized again. We can add check here that the decoder is in an > uninitialized state. Or we can defer this to the CDM to figure out. Added is_initialized_ flag to FFmpegCdmVideoDecoder, and Initialize() will now fail when it's true. http://codereview.chromium.org/10899021/diff/45009/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.cc (right): http://codereview.chromium.org/10899021/diff/45009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.cc:98: static int GetCdmVideoBufferImpl(AVCodecContext* s, AVFrame* frame) { On 2012/10/16 21:59:53, xhwang wrote: > We are mixing anonymous namespace and static functions in this file. Stick to > static since src/media uses it? GetCdmVideoBufferImpl needs to be non-static to be a friend of FFmpegCdmVideoDecoder. I can make it static, but then we have to go back to having FFmpegCdmVideoDecoder::GetVideoBuffer() be a public method. What do you two prefer? http://codereview.chromium.org/10899021/diff/45009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.cc:164: // TODO(tomfinegan): Do we need to copy the extra data? Zeroing the members On 2012/10/16 21:59:53, xhwang wrote: > We don't. extra_data is only useful for initialization. It seems to me that we > are not using config_ anywhere in this file. Remove? We don't need our own copy, but we do need to make a copy for within the AVCodecContext, which is now done. http://codereview.chromium.org/10899021/diff/45009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.cc:177: avcodec_flush_buffers(codec_context_); On 2012/10/16 21:59:53, xhwang wrote: > Do we want to have a flag or state to remember this? After > avcoded_flush_buffers, we should only accept empty inputs. Discussed offline. http://codereview.chromium.org/10899021/diff/45009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.cc:188: for (int i = 0; static_cast<VideoPlane>(i) < kMaxPlanes; ++i) { On 2012/10/16 21:59:53, xhwang wrote: > kMaxPlanes is an int. Why do we need the static_cast here? kMaxPlanes was an int, it's part of VideoPlanes now. http://codereview.chromium.org/10899021/diff/45009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.cc:204: // Due to FFmpeg API changes we no longer have const read-only pointers. On 2012/10/16 21:59:53, xhwang wrote: > put this above line 207? Done, and reworded it. http://codereview.chromium.org/10899021/diff/45009/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.h (right): http://codereview.chromium.org/10899021/diff/45009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.h:36: // false upon failure. On 2012/10/16 21:59:53, xhwang wrote: > During EOS, we'll call DecodeFrame with NULL input until there's no output > available, in which case the return is true and the output is NULL. We should be > able to distinguish this case with NoMoreData. Method returns cdm::Status now, as it should have to begin with. Sorry about that, for some reason I thought CDM::DecryptAndDecodeFrame returned bool. http://codereview.chromium.org/10899021/diff/45009/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/ffmpeg_cdm_video_frame.cc (right): http://codereview.chromium.org/10899021/diff/45009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_video_frame.cc:79: return format_; On 2012/10/16 21:59:53, xhwang wrote: > accessors can be defined in header files Done. Moved the single line ones into the header file. http://codereview.chromium.org/10899021/diff/45009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_video_frame.cc:96: On 2012/10/16 21:59:53, xhwang wrote: > remove extra empty line Done. http://codereview.chromium.org/10899021/diff/45009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_video_frame.cc:103: bool FFmpegCdmVideoFrame::AllocateYUV(cdm::Allocator* allocator) { On 2012/10/16 21:59:53, xhwang wrote: > Bikeshedding: Not sure if it should be YUV or Yuv :) Done. I prefer Yuv anyway. :) http://codereview.chromium.org/10899021/diff/45009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_video_frame.cc:108: DVLOG(1) << "VideoFrame::AllocateYUV: width=" << size_.width On 2012/10/16 21:59:53, xhwang wrote: > s/VideoFame/FFmpegCdmVideoFrame Removed class name from the comment. File name and method is enough. http://codereview.chromium.org/10899021/diff/45009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_video_frame.cc:118: size_t uv_height = format_ == cdm::kYv12 ? y_height / 2 : y_height; On 2012/10/16 21:59:53, xhwang wrote: > The difference b/w YV12 and I420 is that YV12 is YVU but I420 is YUV. In case of > I420, the uv_height should still be y_height / 2, right? Thanks for catching this-- this is based on media::VideoFrame, and its AllocateYUV() supports YV12 and YV16. Done.
A few more comments. I'll defer to xhwang on the new .cc files. http://codereview.chromium.org/10899021/diff/45009/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.h (right): http://codereview.chromium.org/10899021/diff/45009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.h:45: int GetVideoBuffer(AVCodecContext* codec_context, AVFrame* frame); On 2012/10/16 21:44:44, Tom Finegan wrote: > On 2012/10/16 18:04:30, ddorwin wrote: > > Can this be private? It's provided to FFmpeg as a pointer, right? > > This is what GetCdmVideoBufferImpl() calls, and that's why it was public. I've > made GetCdmVideoBufferImpl() a friend, and made it non-static (required to > friend it). This puts GetCdmVideoBufferImpl() in the webkit_media namespace, > while ReleaseCdmBufferImpl() remains static. Declaring the get and release > methods differently seems a little weird, but it should work. > > Should I keep it this way, or change it back? Go back. This causes you to friend and non-static the Impl method, so this is worse. I forgot that FFmpeg doesn't use member functions. http://codereview.chromium.org/10899021/diff/62004/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/clear_key_cdm.cc (right): http://codereview.chromium.org/10899021/diff/62004/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/clear_key_cdm.cc:239: video_decoder_.reset(new webkit_media::FFmpegCdmVideoDecoder(allocator_)); Per discussion in patch 18, shouldn't we only do this if it's null? http://codereview.chromium.org/10899021/diff/62004/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.cc (right): http://codereview.chromium.org/10899021/diff/62004/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.cc:101: int GetCdmVideoBufferImpl(AVCodecContext* s, AVFrame* frame) { "s" is a bad name. Is that consistent with something? Is this how the existing FFVD works? http://codereview.chromium.org/10899021/diff/62004/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.h (right): http://codereview.chromium.org/10899021/diff/62004/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.h:10: #include "base/memory/ref_counted.h" still here http://codereview.chromium.org/10899021/diff/62004/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/ffmpeg_cdm_video_frame.h (right): http://codereview.chromium.org/10899021/diff/62004/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_video_frame.h:40: void MoveVideoFrame(cdm::VideoFrame* target); Export, Extract, Convert, Transfer, ...?
I think I've addressed everything now, PTAL. Thanks! http://codereview.chromium.org/10899021/diff/45009/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.cc (right): http://codereview.chromium.org/10899021/diff/45009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.cc:98: static int GetCdmVideoBufferImpl(AVCodecContext* s, AVFrame* frame) { On 2012/10/17 02:39:10, Tom Finegan wrote: > On 2012/10/16 21:59:53, xhwang wrote: > > We are mixing anonymous namespace and static functions in this file. Stick to > > static since src/media uses it? > > GetCdmVideoBufferImpl needs to be non-static to be a friend of > FFmpegCdmVideoDecoder. I can make it static, but then we have to go back to > having FFmpegCdmVideoDecoder::GetVideoBuffer() be a public method. > > What do you two prefer? Back to static and a public GetVideoBuffer(). http://codereview.chromium.org/10899021/diff/45009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.cc:188: for (int i = 0; static_cast<VideoPlane>(i) < kMaxPlanes; ++i) { On 2012/10/17 02:39:10, Tom Finegan wrote: > On 2012/10/16 21:59:53, xhwang wrote: > > kMaxPlanes is an int. Why do we need the static_cast here? > > kMaxPlanes was an int, it's part of VideoPlanes now. Totally misunderstood this the first time. Fixed in FFmpegCdmVideoFrame::TransferVideoFrame() (I think). http://codereview.chromium.org/10899021/diff/45009/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.h (right): http://codereview.chromium.org/10899021/diff/45009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.h:45: int GetVideoBuffer(AVCodecContext* codec_context, AVFrame* frame); On 2012/10/17 03:18:20, ddorwin wrote: > On 2012/10/16 21:44:44, Tom Finegan wrote: > > On 2012/10/16 18:04:30, ddorwin wrote: > > > Can this be private? It's provided to FFmpeg as a pointer, right? > > > > This is what GetCdmVideoBufferImpl() calls, and that's why it was public. I've > > made GetCdmVideoBufferImpl() a friend, and made it non-static (required to > > friend it). This puts GetCdmVideoBufferImpl() in the webkit_media namespace, > > while ReleaseCdmBufferImpl() remains static. Declaring the get and release > > methods differently seems a little weird, but it should work. > > > > Should I keep it this way, or change it back? > > Go back. This causes you to friend and non-static the Impl method, so this is > worse. I forgot that FFmpeg doesn't use member functions. Done. http://codereview.chromium.org/10899021/diff/62004/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/clear_key_cdm.cc (right): http://codereview.chromium.org/10899021/diff/62004/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/clear_key_cdm.cc:239: video_decoder_.reset(new webkit_media::FFmpegCdmVideoDecoder(allocator_)); On 2012/10/17 03:18:20, ddorwin wrote: > Per discussion in patch 18, shouldn't we only do this if it's null? Thanks, missed it. Done. http://codereview.chromium.org/10899021/diff/62004/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.cc (right): http://codereview.chromium.org/10899021/diff/62004/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.cc:101: int GetCdmVideoBufferImpl(AVCodecContext* s, AVFrame* frame) { On 2012/10/17 03:18:20, ddorwin wrote: > "s" is a bad name. Is that consistent with something? Is this how the existing > FFVD works? From media. Renamed args. http://codereview.chromium.org/10899021/diff/62004/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.h (right): http://codereview.chromium.org/10899021/diff/62004/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.h:10: #include "base/memory/ref_counted.h" On 2012/10/17 03:18:20, ddorwin wrote: > still here But I fixed it in the .cc file... Done. http://codereview.chromium.org/10899021/diff/62004/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/ffmpeg_cdm_video_frame.h (right): http://codereview.chromium.org/10899021/diff/62004/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_video_frame.h:40: void MoveVideoFrame(cdm::VideoFrame* target); On 2012/10/17 03:18:20, ddorwin wrote: > Export, Extract, Convert, Transfer, ...? Transfer works. Done.
LG, just some questions. Wait for xhwang's full review. http://codereview.chromium.org/10899021/diff/66013/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/clear_key_cdm.cc (right): http://codereview.chromium.org/10899021/diff/66013/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/clear_key_cdm.cc:239: if (!video_decoder_) FWIW, we never delete video_decoder_. Should we? I guess this is fine. http://codereview.chromium.org/10899021/diff/66013/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/ffmpeg_cdm_video_frame.cc (right): http://codereview.chromium.org/10899021/diff/66013/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_video_frame.cc:96: void FFmpegCdmVideoFrame::TransferVideoFrame(cdm::VideoFrame* target) { Should we clear this frame to invalid values so it doesn't get used? Or is that overkill? Are there DCHECKs on frame_buffer_ that would cover this?
http://codereview.chromium.org/10899021/diff/45009/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/cdm_wrapper.cc (right): http://codereview.chromium.org/10899021/diff/45009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/cdm_wrapper.cc:621: void CdmWrapper::DeinitializeDecoder(PP_DecryptorStreamType decoder_type, On 2012/10/17 02:39:10, Tom Finegan wrote: > On 2012/10/16 21:59:53, xhwang wrote: > > s/decoder_type/stream_type ? > > I prefer decoder_type here. I think it fits better with DeinitializeDecoder... > if/when we add a stream_id to these methods stream_type might be better. > > I also want the names to match the pepper API, and I used decoder_type > throughout. Do you feel really strongly about this? SG :) http://codereview.chromium.org/10899021/diff/45009/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.cc (right): http://codereview.chromium.org/10899021/diff/45009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.cc:164: // TODO(tomfinegan): Do we need to copy the extra data? Zeroing the members On 2012/10/17 02:39:10, Tom Finegan wrote: > On 2012/10/16 21:59:53, xhwang wrote: > > We don't. extra_data is only useful for initialization. It seems to me that we > > are not using config_ anywhere in this file. Remove? > > We don't need our own copy, but we do need to make a copy for within the > AVCodecContext, which is now done. Yes, so after we make the copy we don't need to save it to config_ anymore, right? remove "config_"? http://codereview.chromium.org/10899021/diff/45009/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.h (right): http://codereview.chromium.org/10899021/diff/45009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.h:50: // FFmpeg structures owned by this object. On 2012/10/16 21:44:44, Tom Finegan wrote: > On 2012/10/16 18:04:30, ddorwin wrote: > > Can we use scoped_ptr since these are owned? Or is that too much of base? > > Discussed offline. Not really too much base, more that we need wrapper classes > for any FFmpeg objects that we want to put in scopers because we must pass > FFmpeg objects to av_free() instead of using delete. I miss boost::shared_ptr where you can specify deleters :) http://codereview.chromium.org/10899021/diff/66013/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/cdm_wrapper.cc (right): http://codereview.chromium.org/10899021/diff/66013/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/cdm_wrapper.cc:798: // TODO(xhwang): Handle cdm::kNeedMoreData. thanks! http://codereview.chromium.org/10899021/diff/66013/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.cc (right): http://codereview.chromium.org/10899021/diff/66013/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.cc:19: #include <libavcodec/avcodec.h> should we use "" instead of <>? http://codereview.chromium.org/10899021/diff/66013/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.cc:78: memset(codec_context->extradata + config.extra_data_size, '\0', Not sure about ffmpeg, but s/'\0'/0 since it's binary data not text? http://codereview.chromium.org/10899021/diff/66013/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.cc:99: } // namespace Can we make all functions above static instead of defining them in an anonymous namespace? http://codereview.chromium.org/10899021/diff/66013/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.cc:242: // 2) End of stream was reached and all internal frames have been output As you mentioned offline, we can handle NeedMoreData upstream in CdmWrapper, or in DVD. Maybe we can remove NeedMoreData all together later. But for now we are relying on the CDM to return: - For 1): NeedMoreData with NULL output frame - For 2) Success for with NULL output frame Could you please make this change for now. I'll figure out how we can best do this later. http://codereview.chromium.org/10899021/diff/66013/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.cc:329: is_initialized_ = false; Move this to Deinitialize()? http://codereview.chromium.org/10899021/diff/66013/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.h (right): http://codereview.chromium.org/10899021/diff/66013/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.h:17: static const uint32_t kBufferAlignment = 32; Move this to ffmpeg_cdm_video_frame.cc ? http://codereview.chromium.org/10899021/diff/66013/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.h:18: static const uint32_t kBufferPadBytes = kBufferAlignment - 1; Where is this used? http://codereview.chromium.org/10899021/diff/66013/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.h:19: static const int kDecodeThreads = 1; move this to ffmpeg_cdm_video_decoder.cc file? http://codereview.chromium.org/10899021/diff/66013/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/ffmpeg_cdm_video_frame.cc (right): http://codereview.chromium.org/10899021/diff/66013/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_video_frame.cc:12: remove extra empty line http://codereview.chromium.org/10899021/diff/66013/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_video_frame.cc:17: inline size_t RoundUp(size_t value, size_t alignment) { Can we use "int" ? http://codereview.chromium.org/10899021/diff/66013/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_video_frame.cc:39: } // namespace media code prefer using static to anonymous namespace for historical reason. The so only rule you are violating is "be consistent with existing code". :) http://codereview.chromium.org/10899021/diff/66013/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_video_frame.cc:87: bool FFmpegCdmVideoFrame::IsValidConfig(cdm::VideoFormat format, This function is not using any class members. Make is a file scope static function in this file instead of a class static function. http://codereview.chromium.org/10899021/diff/66013/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_video_frame.cc:124: kBufferAlignment); So for Y, stride is RoundUp(width, kBufferAlignment). For U and V, stride is RoundUp(RoundUp(width, 2) / 2, kBufferAlignment) Why do we need to round-up twice for UV plane? If we do need it, add comments here. http://codereview.chromium.org/10899021/diff/66013/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_video_frame.cc:126: size_t y_height = RoundUp(size_.height, kBufferAlignment); What exactly is kBufferAlignment (==32 for now)? Why do we need to round up height? This could add up to 31 empty rows. It seems to me all rows are properly aligned so the whole Y/U/V planes are all aligned already. http://codereview.chromium.org/10899021/diff/66013/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/ffmpeg_cdm_video_frame.h (right): http://codereview.chromium.org/10899021/diff/66013/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_video_frame.h:40: void TransferVideoFrame(cdm::VideoFrame* target); nit: s/Transfer/Pass? We use "pass/take" in chrome for ownership transfer. But it's really up to you.
I think I got to everything. PTAL, thanks! http://codereview.chromium.org/10899021/diff/66013/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/clear_key_cdm.cc (right): http://codereview.chromium.org/10899021/diff/66013/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/clear_key_cdm.cc:239: if (!video_decoder_) On 2012/10/17 06:10:19, ddorwin wrote: > FWIW, we never delete video_decoder_. Should we? I guess this is fine. Doesn't seem necessary with the scoper. http://codereview.chromium.org/10899021/diff/66013/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.cc (right): http://codereview.chromium.org/10899021/diff/66013/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.cc:19: #include <libavcodec/avcodec.h> On 2012/10/17 18:43:00, xhwang wrote: > should we use "" instead of <>? This is consistent with media, and I think it's required if the build is set to use system ffmpeg. http://codereview.chromium.org/10899021/diff/66013/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.cc:78: memset(codec_context->extradata + config.extra_data_size, '\0', On 2012/10/17 18:43:00, xhwang wrote: > Not sure about ffmpeg, but s/'\0'/0 since it's binary data not text? This was copy/pasted from media... Done. http://codereview.chromium.org/10899021/diff/66013/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.cc:99: } // namespace On 2012/10/17 18:43:00, xhwang wrote: > Can we make all functions above static instead of defining them in an anonymous > namespace? Done. http://codereview.chromium.org/10899021/diff/66013/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.cc:242: // 2) End of stream was reached and all internal frames have been output On 2012/10/17 18:43:00, xhwang wrote: > As you mentioned offline, we can handle NeedMoreData upstream in CdmWrapper, or > in DVD. Maybe we can remove NeedMoreData all together later. But for now we are > relying on the CDM to return: > - For 1): NeedMoreData with NULL output frame > - For 2) Success for with NULL output frame > > Could you please make this change for now. I'll figure out how we can best do > this later. I think I did what your comment meant: 1) DecodeFrame() returns kNeedMoreData when there is non-null input, and no output is produced. 2) DecodeFrame() returns kSuccess when there is null input, and no output is produced. PTAL at the comments. http://codereview.chromium.org/10899021/diff/66013/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.cc:329: is_initialized_ = false; On 2012/10/17 18:43:00, xhwang wrote: > Move this to Deinitialize()? Done. http://codereview.chromium.org/10899021/diff/66013/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.h (right): http://codereview.chromium.org/10899021/diff/66013/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.h:17: static const uint32_t kBufferAlignment = 32; On 2012/10/17 18:43:00, xhwang wrote: > Move this to ffmpeg_cdm_video_frame.cc ? Done. http://codereview.chromium.org/10899021/diff/66013/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.h:18: static const uint32_t kBufferPadBytes = kBufferAlignment - 1; On 2012/10/17 18:43:00, xhwang wrote: > Where is this used? From media, done. http://codereview.chromium.org/10899021/diff/66013/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.h:19: static const int kDecodeThreads = 1; On 2012/10/17 18:43:00, xhwang wrote: > move this to ffmpeg_cdm_video_decoder.cc file? Done. http://codereview.chromium.org/10899021/diff/66013/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.h:19: static const int kDecodeThreads = 1; On 2012/10/17 18:43:00, xhwang wrote: > move this to ffmpeg_cdm_video_decoder.cc file? Done. http://codereview.chromium.org/10899021/diff/66013/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/ffmpeg_cdm_video_frame.cc (right): http://codereview.chromium.org/10899021/diff/66013/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_video_frame.cc:12: On 2012/10/17 18:43:00, xhwang wrote: > remove extra empty line Done. http://codereview.chromium.org/10899021/diff/66013/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_video_frame.cc:17: inline size_t RoundUp(size_t value, size_t alignment) { On 2012/10/17 18:43:00, xhwang wrote: > Can we use "int" ? Done. http://codereview.chromium.org/10899021/diff/66013/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_video_frame.cc:39: } // namespace On 2012/10/17 18:43:00, xhwang wrote: > media code prefer using static to anonymous namespace for historical reason. The > so only rule you are violating is "be consistent with existing code". :) Removed anonymous namespace, and made things static. http://codereview.chromium.org/10899021/diff/66013/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_video_frame.cc:87: bool FFmpegCdmVideoFrame::IsValidConfig(cdm::VideoFormat format, On 2012/10/17 18:43:00, xhwang wrote: > This function is not using any class members. Make is a file scope static > function in this file instead of a class static function. Can't do that-- this method is called from in this file, and from in ffmpeg_cdm_video_decoder.cc. Well, I can if you want me to make two copies of the function and declare it in ffmpeg_cdm_video_frame.h, but that doesn't seem right. http://codereview.chromium.org/10899021/diff/66013/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_video_frame.cc:96: void FFmpegCdmVideoFrame::TransferVideoFrame(cdm::VideoFrame* target) { On 2012/10/17 06:10:19, ddorwin wrote: > Should we clear this frame to invalid values so it doesn't get used? Or is that > overkill? Are there DCHECKs on frame_buffer_ that would cover this? There's a DCHECK on frame_buffer() in FFmpegCdmVideoDecoder::DecodeFrame() when ffmpeg says gives us a non-zero framed_decoded. Invalidating the values doesn't seem like overkill. Done. Also, added code setting the missing values from cdm::VideoFrame. http://codereview.chromium.org/10899021/diff/66013/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_video_frame.cc:124: kBufferAlignment); On 2012/10/17 18:43:00, xhwang wrote: > So for Y, stride is RoundUp(width, kBufferAlignment). > For U and V, stride is > RoundUp(RoundUp(width, 2) / 2, kBufferAlignment) > Why do we need to round-up twice for UV plane? If we do need it, add comments > here. I think it's in case width is an odd number, which isn't strictly valid for YV12/I420, but can happen. This is copied from media, and there's no explanation there. I'm not certain of the problem it originally solved beyond what I've said above. I prefer keeping this as it is in media. http://codereview.chromium.org/10899021/diff/66013/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_video_frame.cc:126: size_t y_height = RoundUp(size_.height, kBufferAlignment); On 2012/10/17 18:43:00, xhwang wrote: > What exactly is kBufferAlignment (==32 for now)? Why do we need to round up > height? This could add up to 31 empty rows. It seems to me all rows are properly > aligned so the whole Y/U/V planes are all aligned already. Added comment from media that explains this. http://codereview.chromium.org/10899021/diff/66013/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/ffmpeg_cdm_video_frame.h (right): http://codereview.chromium.org/10899021/diff/66013/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_video_frame.h:40: void TransferVideoFrame(cdm::VideoFrame* target); On 2012/10/17 18:43:00, xhwang wrote: > nit: s/Transfer/Pass? We use "pass/take" in chrome for ownership transfer. But > it's really up to you. Used Pass.
http://codereview.chromium.org/10899021/diff/66013/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/ffmpeg_cdm_video_frame.cc (right): http://codereview.chromium.org/10899021/diff/66013/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_video_frame.cc:87: bool FFmpegCdmVideoFrame::IsValidConfig(cdm::VideoFormat format, On 2012/10/17 19:55:44, Tom Finegan wrote: > On 2012/10/17 18:43:00, xhwang wrote: > > This function is not using any class members. Make is a file scope static > > function in this file instead of a class static function. > > Can't do that-- this method is called from in this file, and from in > ffmpeg_cdm_video_decoder.cc. > > Well, I can if you want me to make two copies of the function and declare it in > ffmpeg_cdm_video_frame.h, but that doesn't seem right. Ok, did't notice it's used in another file. In that case, make it a standalone non-class non-static function? http://codereview.chromium.org/10899021/diff/66013/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_video_frame.cc:124: kBufferAlignment); On 2012/10/17 19:55:44, Tom Finegan wrote: > On 2012/10/17 18:43:00, xhwang wrote: > > So for Y, stride is RoundUp(width, kBufferAlignment). > > For U and V, stride is > > RoundUp(RoundUp(width, 2) / 2, kBufferAlignment) > > Why do we need to round-up twice for UV plane? If we do need it, add comments > > here. > > I think it's in case width is an odd number, which isn't strictly valid for > YV12/I420, but can happen. > > This is copied from media, and there's no explanation there. I'm not certain of > the problem it originally solved beyond what I've said above. I prefer keeping > this as it is in media. Since kBufferAlignment is even, I don't think it's necessary. But I agree we can just keep it as it is in media now. http://codereview.chromium.org/10899021/diff/67004/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.cc (right): http://codereview.chromium.org/10899021/diff/67004/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.cc:177: config_ = config; Copy from previous review: Yes, so after we make the copy we don't need to save it to config_ anymore, right? remove "config_"? http://codereview.chromium.org/10899021/diff/67004/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.cc:252: } Thanks. We probably can remove kNeedMoreData later altogether. Bug for tracking this: crbug.com/156425 http://codereview.chromium.org/10899021/diff/67004/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/ffmpeg_cdm_video_frame.cc (right): http://codereview.chromium.org/10899021/diff/67004/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_video_frame.cc:133: // (vertically). See libavcodec/utils.c:avcodec_align_dimensions2(). good to know! Thanks.
LG wrt my comments. I'll let xhwang wrap up the review.
I think everything is resolved with the exception of FFmpegCdmVideoFrame::IsValidConfig(). PTAL at comment. Thanks! http://codereview.chromium.org/10899021/diff/66013/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/ffmpeg_cdm_video_frame.cc (right): http://codereview.chromium.org/10899021/diff/66013/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_video_frame.cc:87: bool FFmpegCdmVideoFrame::IsValidConfig(cdm::VideoFormat format, On 2012/10/17 20:23:10, xhwang wrote: > On 2012/10/17 19:55:44, Tom Finegan wrote: > > On 2012/10/17 18:43:00, xhwang wrote: > > > This function is not using any class members. Make is a file scope static > > > function in this file instead of a class static function. > > > > Can't do that-- this method is called from in this file, and from in > > ffmpeg_cdm_video_decoder.cc. > > > > Well, I can if you want me to make two copies of the function and declare it > in > > ffmpeg_cdm_video_frame.h, but that doesn't seem right. > > Ok, did't notice it's used in another file. In that case, make it a standalone > non-class non-static function? I made it a static member function of FFmpegCdmVideoFrame for two reasons: - This is what media::VideoFrame does (and it has the same characteristic of not touching class members, but I think that's true for a lot of static member functions) - I don't want to put a method named IsValidConfig() in the webkit_media namespace The latter could be solved with naming it IsValidVideoFrameConfig(), but then we're back to it looking like it should be a member of FFmpegCdmVideoFrame. I guess all that I really want to know about this is why you want it changed. Can we just leave this as-is? http://codereview.chromium.org/10899021/diff/66013/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_video_frame.cc:124: kBufferAlignment); On 2012/10/17 20:23:10, xhwang wrote: > On 2012/10/17 19:55:44, Tom Finegan wrote: > > On 2012/10/17 18:43:00, xhwang wrote: > > > So for Y, stride is RoundUp(width, kBufferAlignment). > > > For U and V, stride is > > > RoundUp(RoundUp(width, 2) / 2, kBufferAlignment) > > > Why do we need to round-up twice for UV plane? If we do need it, add > comments > > > here. > > > > I think it's in case width is an odd number, which isn't strictly valid for > > YV12/I420, but can happen. > > > > This is copied from media, and there's no explanation there. I'm not certain > of > > the problem it originally solved beyond what I've said above. I prefer keeping > > this as it is in media. > > Since kBufferAlignment is even, I don't think it's necessary. But I agree we can > just keep it as it is in media now. sgtm http://codereview.chromium.org/10899021/diff/67004/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.cc (right): http://codereview.chromium.org/10899021/diff/67004/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.cc:177: config_ = config; On 2012/10/17 20:23:10, xhwang wrote: > Copy from previous review: > Yes, so after we make the copy we don't need to save it to config_ anymore, > right? remove "config_"? Right, and done, really this time. I kind of suck at dealing with comments spread over multiple patchsets. :) http://codereview.chromium.org/10899021/diff/67004/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.cc:252: } On 2012/10/17 20:23:10, xhwang wrote: > Thanks. We probably can remove kNeedMoreData later altogether. Bug for tracking > this: crbug.com/156425 sgtm
lgtm Thanks for the hard work! Did you test this patch? You can pull my plugin instance patch (11091005) to test this.
Removed use of get_buffer and release_buffer from AVCodecContext, and changed the way output video buffers are allocated. PTAL, thanks!
This is a good news/bad news update to the CL. Bad news: PTAL again, enough changed that I think it's needed. The main things are CopyAvFrameTo() in FFmpegCdmVideoDecoder, and the removal of FFmpegCdmVideoFrame. Good news: DecryptAndDecode works with the current version of the CL. Yay. \o/ It sucks that we have to do the extra copy, but I don't think there's an alternative outside of letting ffmpeg own a reference on the PPB_Buffer wrapper in cdm::Buffer, which sounds like a bad idea.
One extra copy of decoded frame should be relatively cheap compared to decoding. Also our clear key CDM is mostly for testing purpose, so I think it's okay. The new patch looks good in general. Just a few comments and questions. http://codereview.chromium.org/10899021/diff/63034/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/cdm_wrapper.cc (right): http://codereview.chromium.org/10899021/diff/63034/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/cdm_wrapper.cc:829: decrypted_frame_info.result = PP_DECRYPTRESULT_SUCCESS; I am surprised to see we don't have NeedMoreData in PP_DecryptResult :) But that's okay. I'll let DVD handle it and probably remove the need for NeedMoreData altogether. http://codereview.chromium.org/10899021/diff/63034/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.cc (right): http://codereview.chromium.org/10899021/diff/63034/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.cc:145: codec_context_->flags |= CODEC_FLAG_EMU_EDGE; So without setting "get_buffer" here, are we using the default memory allocation in ffmpeg? http://codereview.chromium.org/10899021/diff/63034/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.cc:280: const int uv_stride = av_frame_->width / 2; Do we ensure av_frame_->width is even? http://codereview.chromium.org/10899021/diff/63034/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.h (right): http://codereview.chromium.org/10899021/diff/63034/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.h:10: #include "base/memory/scoped_ptr.h" why scoped_ptr here? http://codereview.chromium.org/10899021/diff/63034/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.h:28: static bool IsValidOutputConfig(cdm::VideoFormat format, const-ref for format?
BTW, YAY! for D&D working :)
Addressed comments. Looking at the test failures now. http://codereview.chromium.org/10899021/diff/63034/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/cdm_wrapper.cc (right): http://codereview.chromium.org/10899021/diff/63034/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/cdm_wrapper.cc:829: decrypted_frame_info.result = PP_DECRYPTRESULT_SUCCESS; On 2012/10/18 16:30:25, xhwang wrote: > I am surprised to see we don't have NeedMoreData in PP_DecryptResult :) But > that's okay. I'll let DVD handle it and probably remove the need for > NeedMoreData altogether. sgtm http://codereview.chromium.org/10899021/diff/63034/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.cc (right): http://codereview.chromium.org/10899021/diff/63034/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.cc:145: codec_context_->flags |= CODEC_FLAG_EMU_EDGE; On 2012/10/18 16:30:25, xhwang wrote: > So without setting "get_buffer" here, are we using the default memory allocation > in ffmpeg? Yes. http://codereview.chromium.org/10899021/diff/63034/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.cc:280: const int uv_stride = av_frame_->width / 2; On 2012/10/18 16:30:25, xhwang wrote: > Do we ensure av_frame_->width is even? I do now. IsValidOutputConfig() will fail, and there's a DCHECK in this method. http://codereview.chromium.org/10899021/diff/63034/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.h (right): http://codereview.chromium.org/10899021/diff/63034/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.h:10: #include "base/memory/scoped_ptr.h" On 2012/10/18 16:30:25, xhwang wrote: > why scoped_ptr here? Forgot to remove it, thanks. http://codereview.chromium.org/10899021/diff/63034/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.h:28: static bool IsValidOutputConfig(cdm::VideoFormat format, On 2012/10/18 16:30:25, xhwang wrote: > const-ref for format? It's just an enum?
looks good now. please fix the tests. http://codereview.chromium.org/10899021/diff/63034/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.h (right): http://codereview.chromium.org/10899021/diff/63034/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.h:28: static bool IsValidOutputConfig(cdm::VideoFormat format, On 2012/10/18 19:29:41, Tom Finegan wrote: > On 2012/10/18 16:30:25, xhwang wrote: > > const-ref for format? > > It's just an enum? nm then.
Reviewers, PTAL. This is now passing tests locally. Pepper owners are now TBR'd in the description, but if you guys have time it would be great to have an OWNERS LGTM. Thanks!
(still lgtm) % discussion about using zero PP_Resource. http://codereview.chromium.org/10899021/diff/73040/ppapi/proxy/ppp_content_de... File ppapi/proxy/ppp_content_decryptor_private_proxy.cc (right): http://codereview.chromium.org/10899021/diff/73040/ppapi/proxy/ppp_content_de... ppapi/proxy/ppp_content_decryptor_private_proxy.cc:488: PP_Resource plugin_resource = 0; Since PP_Resource of 0 can happen if error happens, I feel our implementation isn't robust. I wonder if we can add extra info in block info to indicate that input or output is empty. Not needed for this CL at all, but want to start discussion on this.
http://codereview.chromium.org/10899021/diff/73040/ppapi/proxy/ppp_content_de... File ppapi/proxy/ppp_content_decryptor_private_proxy.cc (right): http://codereview.chromium.org/10899021/diff/73040/ppapi/proxy/ppp_content_de... ppapi/proxy/ppp_content_decryptor_private_proxy.cc:491: PPB_Buffer_Proxy::AddProxyResource(encrypted_buffer.resource, If you don't hit this line I believe encrypted_buffer.resource will leak, causing the resource object in the renderer to leak. http://codereview.chromium.org/10899021/diff/73040/ppapi/proxy/ppp_content_de... ppapi/proxy/ppp_content_decryptor_private_proxy.cc:498: return; If you hit this the plugin_resource will leak
Fixed the leaks, and turned the video decoder back on... still fails on linux bots, but works locally. Looking at it... http://codereview.chromium.org/10899021/diff/73040/ppapi/proxy/ppp_content_de... File ppapi/proxy/ppp_content_decryptor_private_proxy.cc (right): http://codereview.chromium.org/10899021/diff/73040/ppapi/proxy/ppp_content_de... ppapi/proxy/ppp_content_decryptor_private_proxy.cc:488: PP_Resource plugin_resource = 0; On 2012/10/19 07:12:40, xhwang wrote: > Since PP_Resource of 0 can happen if error happens, I feel our implementation > isn't robust. I wonder if we can add extra info in block info to indicate that > input or output is empty. Not needed for this CL at all, but want to start > discussion on this. Discussion in progress offline, but I don't think we have to do that... http://codereview.chromium.org/10899021/diff/73040/ppapi/proxy/ppp_content_de... ppapi/proxy/ppp_content_decryptor_private_proxy.cc:491: PPB_Buffer_Proxy::AddProxyResource(encrypted_buffer.resource, On 2012/10/19 23:03:54, brettw wrote: > If you don't hit this line I believe encrypted_buffer.resource will leak, > causing the resource object in the renderer to leak. Changed this and the usage above to occur only when there's a non-zero encrypted_buffer.resource. http://codereview.chromium.org/10899021/diff/73040/ppapi/proxy/ppp_content_de... ppapi/proxy/ppp_content_decryptor_private_proxy.cc:498: return; On 2012/10/19 23:03:54, brettw wrote: > If you hit this the plugin_resource will leak Done. Thanks! This part now happens before we do anything with the resource. Hopefully it's enough to avoid the problem, anyway.
If you want to refactor the initialization in a separate CL, that's fine, but we need to fix it. http://codereview.chromium.org/10899021/diff/100019/webkit/media/crypto/ppapi... File webkit/media/crypto/ppapi/clear_key_cdm.cc (right): http://codereview.chromium.org/10899021/diff/100019/webkit/media/crypto/ppapi... webkit/media/crypto/ppapi/clear_key_cdm.cc:73: DVLOG(1) << "CreateCdmInstance()"; This should all only be done once per module. That's why it should be invoked as part of the ppapi module initialization. "Module* CreateModule()" should call cdm.h::InitializeCdmModule() (global, just like CreateCdmInstance()), either directly or indirectly via some CdmWrapperModule method. http://codereview.chromium.org/10899021/diff/100019/webkit/media/crypto/ppapi... File webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.cc (right): http://codereview.chromium.org/10899021/diff/100019/webkit/media/crypto/ppapi... webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.cc:50: if (video_codec == cdm::VideoDecoderConfig::kCodecVP8) Why did you change from the switch statement? (The previous implementation should have had NOTREACHED and return in default with no return at the end.)
On 2012/10/21 22:10:53, ddorwin wrote: > If you want to refactor the initialization in a separate CL, that's fine, but we > need to fix it. Actually, InitializeMediaLibrary() has a check for whether it's been initialized, so this is probably fine. I still think an explicit call would be better long term.
http://codereview.chromium.org/10899021/diff/100019/webkit/media/crypto/ppapi... File webkit/media/crypto/ppapi/clear_key_cdm.cc (right): http://codereview.chromium.org/10899021/diff/100019/webkit/media/crypto/ppapi... webkit/media/crypto/ppapi/clear_key_cdm.cc:73: DVLOG(1) << "CreateCdmInstance()"; On 2012/10/21 22:10:53, ddorwin wrote: > This should all only be done once per module. That's why it should be invoked as > part of the ppapi module initialization. > "Module* CreateModule()" should call cdm.h::InitializeCdmModule() (global, just > like CreateCdmInstance()), either directly or indirectly via some > CdmWrapperModule method. Discussed offline. Is this way OK, or do you want me to add InitializeCdmModule() before trying to land this? http://codereview.chromium.org/10899021/diff/100019/webkit/media/crypto/ppapi... File webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.cc (right): http://codereview.chromium.org/10899021/diff/100019/webkit/media/crypto/ppapi... webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.cc:50: if (video_codec == cdm::VideoDecoderConfig::kCodecVP8) On 2012/10/21 22:10:53, ddorwin wrote: > Why did you change from the switch statement? > (The previous implementation should have had NOTREACHED and return in default > with no return at the end.) Because it felt like 4 lines of code to read that weren't really doing anything. This class supports only VP8 anyway, why bother with the switch and default?
http://codereview.chromium.org/10899021/diff/100019/webkit/media/crypto/ppapi... File webkit/media/crypto/ppapi/clear_key_cdm.cc (right): http://codereview.chromium.org/10899021/diff/100019/webkit/media/crypto/ppapi... webkit/media/crypto/ppapi/clear_key_cdm.cc:73: DVLOG(1) << "CreateCdmInstance()"; On 2012/10/22 02:34:58, Tom Finegan wrote: > On 2012/10/21 22:10:53, ddorwin wrote: > > This should all only be done once per module. That's why it should be invoked > as > > part of the ppapi module initialization. > > "Module* CreateModule()" should call cdm.h::InitializeCdmModule() (global, > just > > like CreateCdmInstance()), either directly or indirectly via some > > CdmWrapperModule method. > > Discussed offline. Is this way OK, or do you want me to add > InitializeCdmModule() before trying to land this? It's up to you. I'm fine with landing this then modifying CDM.h in a separate CL. http://codereview.chromium.org/10899021/diff/100019/webkit/media/crypto/ppapi... File webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.cc (right): http://codereview.chromium.org/10899021/diff/100019/webkit/media/crypto/ppapi... webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.cc:50: if (video_codec == cdm::VideoDecoderConfig::kCodecVP8) On 2012/10/22 02:34:58, Tom Finegan wrote: > On 2012/10/21 22:10:53, ddorwin wrote: > > Why did you change from the switch statement? > > (The previous implementation should have had NOTREACHED and return in default > > with no return at the end.) > > Because it felt like 4 lines of code to read that weren't really doing anything. > This class supports only VP8 anyway, why bother with the switch and default? Okay. We should eventually support more codecs and then it might make sense to go back. Note also that some compiler configs will warn when a switch is missing.
Please take one last look... I would like to CQ this is it makes it through the try run. Thanks!
still lgtm % nits http://codereview.chromium.org/10899021/diff/100023/webkit/media/crypto/ppapi... File webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.cc (right): http://codereview.chromium.org/10899021/diff/100023/webkit/media/crypto/ppapi... webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.cc:50: if (video_codec == cdm::VideoDecoderConfig::kCodecVP8) nit: Also use switch/case to be consistent w/ the above two functions? http://codereview.chromium.org/10899021/diff/100023/webkit/media/crypto/ppapi... webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.cc:235: return cdm::kSuccess; FYI: We'll return cdm::kNeedMoreData for both cases after 11234019 is landed.
http://codereview.chromium.org/10899021/diff/100023/webkit/media/crypto/ppapi... File webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.cc (right): http://codereview.chromium.org/10899021/diff/100023/webkit/media/crypto/ppapi... webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.cc:50: if (video_codec == cdm::VideoDecoderConfig::kCodecVP8) On 2012/10/22 23:03:10, xhwang wrote: > nit: Also use switch/case to be consistent w/ the above two functions? I give, changed back to switch/default. http://codereview.chromium.org/10899021/diff/100023/webkit/media/crypto/ppapi... webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.cc:235: return cdm::kSuccess; On 2012/10/22 23:03:10, xhwang wrote: > FYI: We'll return cdm::kNeedMoreData for both cases after 11234019 is landed. OK.
Mostly comments about non-FFmpeg platforms. I think we can remove a line now, though. http://codereview.chromium.org/10899021/diff/100023/webkit/media/crypto/ppapi... File webkit/media/crypto/ppapi/clear_key_cdm.cc (right): http://codereview.chromium.org/10899021/diff/100023/webkit/media/crypto/ppapi... webkit/media/crypto/ppapi/clear_key_cdm.cc:86: CHECK(g_cdm_module_initialized); I don't think we need this. http://codereview.chromium.org/10899021/diff/100023/webkit/media/crypto/ppapi... webkit/media/crypto/ppapi/clear_key_cdm.cc:267: #if !defined(CLEAR_KEY_CDM_USE_FAKE_VIDEO_DECODER) The correct fix for non-FFmpeg-using platforms is to have #if defined(USE_FFMPEG_DECODER) ... #elseif CLEAR_KEY_CDM_USE_FAKE_VIDEO_DECODER ... #else NOTIMPLEMENTED(); return cdm::kSessionError; #endif You'd need to undefine USE_FFMPEG_DECODER #if defined(CLEAR_KEY_CDM_USE_FAKE_VIDEO_DECODER) http://codereview.chromium.org/10899021/diff/100023/webkit/media/crypto/ppapi... webkit/media/crypto/ppapi/clear_key_cdm.cc:280: #if !defined(CLEAR_KEY_CDM_USE_FAKE_VIDEO_DECODER) These would be #if defined(ENABLE_DECODE_SUPPORT), which would only be set #if defined(USE_FFMPEG_DECODER). In the future, non-FFMPEG platforms could set it too.
http://codereview.chromium.org/10899021/diff/100023/webkit/media/crypto/ppapi... File webkit/media/crypto/ppapi/clear_key_cdm.cc (right): http://codereview.chromium.org/10899021/diff/100023/webkit/media/crypto/ppapi... webkit/media/crypto/ppapi/clear_key_cdm.cc:86: CHECK(g_cdm_module_initialized); On 2012/10/22 23:31:35, ddorwin wrote: > I don't think we need this. Done. http://codereview.chromium.org/10899021/diff/100023/webkit/media/crypto/ppapi... webkit/media/crypto/ppapi/clear_key_cdm.cc:267: #if !defined(CLEAR_KEY_CDM_USE_FAKE_VIDEO_DECODER) On 2012/10/22 23:31:35, ddorwin wrote: > The correct fix for non-FFmpeg-using platforms is to have > #if defined(USE_FFMPEG_DECODER) > ... > #elseif CLEAR_KEY_CDM_USE_FAKE_VIDEO_DECODER > ... > #else > NOTIMPLEMENTED(); > return cdm::kSessionError; > #endif > > You'd need to undefine USE_FFMPEG_DECODER #if > defined(CLEAR_KEY_CDM_USE_FAKE_VIDEO_DECODER) Done. http://codereview.chromium.org/10899021/diff/100023/webkit/media/crypto/ppapi... webkit/media/crypto/ppapi/clear_key_cdm.cc:280: #if !defined(CLEAR_KEY_CDM_USE_FAKE_VIDEO_DECODER) On 2012/10/22 23:31:35, ddorwin wrote: > These would be #if defined(ENABLE_DECODE_SUPPORT), which would only be set #if > defined(USE_FFMPEG_DECODER). > In the future, non-FFMPEG platforms could set it too. Didn't do this yet. Ok to leave for another CL?
http://codereview.chromium.org/10899021/diff/97011/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/clear_key_cdm.cc (right): http://codereview.chromium.org/10899021/diff/97011/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/clear_key_cdm.cc:19: #include "webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.h" I think 9, 10, 13, and 16 go in here too. http://codereview.chromium.org/10899021/diff/97011/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/clear_key_cdm.cc:20: #endif Move to line 43 then put line 22 after it. http://codereview.chromium.org/10899021/diff/97011/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/clear_key_cdm.cc:285: #elif defined(CLEAR_KEY_CDM_USE_FAKE_VIDEO_DECODER) This code will never get hit on FFmpeg platforms. Either each of these needs to check both FAKE and FFMPEG or you need to undefine FFMPEG in the .h file below where FAKE can be uncommented. http://codereview.chromium.org/10899021/diff/97011/webkit/media/webkit_media.... File webkit/media/webkit_media.gypi (right): http://codereview.chromium.org/10899021/diff/97011/webkit/media/webkit_media.... webkit/media/webkit_media.gypi:106: ['OS == "android" or OS == "ios"', { indent 2 more http://codereview.chromium.org/10899021/diff/97011/webkit/media/webkit_media.... webkit/media/webkit_media.gypi:108: 'use_ffmpeg%': 0, I think we should just do the OS check here then separately have the use_ffmpeg condition below. See http://code.google.com/searchframe#OAMlx_jo-ck/src/media/media.gyp&exact_pack.... http://codereview.chromium.org/10899021/diff/97011/webkit/media/webkit_media.... webkit/media/webkit_media.gypi:109: 'defines': ['CLEAR_KEY_CDM_USE_FAKE_VIDEO_DECODER'], Remove. We should just return error in InitializeDecoder, which will cause Decrypt-only to be used. Right xhwang? http://codereview.chromium.org/10899021/diff/97011/webkit/media/webkit_media.... webkit/media/webkit_media.gypi:147: ['os_posix == 1 and OS != "mac" and OS != "ios" and OS != "android"', { Do you still need this? Shouldn't it all be fixed now?
http://codereview.chromium.org/10899021/diff/97011/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/clear_key_cdm.cc (right): http://codereview.chromium.org/10899021/diff/97011/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/clear_key_cdm.cc:19: #include "webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.h" On 2012/10/23 00:32:30, ddorwin wrote: > I think 9, 10, 13, and 16 go in here too. Done. http://codereview.chromium.org/10899021/diff/97011/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/clear_key_cdm.cc:20: #endif On 2012/10/23 00:32:30, ddorwin wrote: > Move to line 43 then put line 22 after it. Done. http://codereview.chromium.org/10899021/diff/97011/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/clear_key_cdm.cc:285: #elif defined(CLEAR_KEY_CDM_USE_FAKE_VIDEO_DECODER) On 2012/10/23 00:32:30, ddorwin wrote: > This code will never get hit on FFmpeg platforms. > > Either each of these needs to check both FAKE and FFMPEG or you need to undefine > FFMPEG in the .h file below where FAKE can be uncommented. Added ifdef in clear_key_cdm.h that undefs USE_FFMPEG_DECODER. http://codereview.chromium.org/10899021/diff/97011/webkit/media/webkit_media.... File webkit/media/webkit_media.gypi (right): http://codereview.chromium.org/10899021/diff/97011/webkit/media/webkit_media.... webkit/media/webkit_media.gypi:106: ['OS == "android" or OS == "ios"', { On 2012/10/23 00:32:30, ddorwin wrote: > indent 2 more Done. http://codereview.chromium.org/10899021/diff/97011/webkit/media/webkit_media.... webkit/media/webkit_media.gypi:108: 'use_ffmpeg%': 0, On 2012/10/23 00:32:30, ddorwin wrote: > I think we should just do the OS check here then separately have the use_ffmpeg > condition below. See > http://code.google.com/searchframe#OAMlx_jo-ck/src/media/media.gyp&exact_pack.... I might have been doing something wrong, but to use the use_ffmpeg variable I had to define it in a variables section at the top of this file instead of down here. Otherwise it was undefined (can't use a variable defined in a conditional in the same conditional in which it was defined?). It's the same place that media.gyp has it. http://codereview.chromium.org/10899021/diff/97011/webkit/media/webkit_media.... webkit/media/webkit_media.gypi:109: 'defines': ['CLEAR_KEY_CDM_USE_FAKE_VIDEO_DECODER'], On 2012/10/23 00:32:30, ddorwin wrote: > Remove. We should just return error in InitializeDecoder, which will cause > Decrypt-only to be used. Right xhwang? Done. http://codereview.chromium.org/10899021/diff/97011/webkit/media/webkit_media.... webkit/media/webkit_media.gypi:147: ['os_posix == 1 and OS != "mac" and OS != "ios" and OS != "android"', { On 2012/10/23 00:32:30, ddorwin wrote: > Do you still need this? Shouldn't it all be fixed now? Done. Forgot to remove.
Build changes LG. Thanks! http://codereview.chromium.org/10899021/diff/97011/webkit/media/webkit_media.... File webkit/media/webkit_media.gypi (right): http://codereview.chromium.org/10899021/diff/97011/webkit/media/webkit_media.... webkit/media/webkit_media.gypi:108: 'use_ffmpeg%': 0, On 2012/10/23 00:54:41, Tom Finegan wrote: > On 2012/10/23 00:32:30, ddorwin wrote: > > I think we should just do the OS check here then separately have the > use_ffmpeg > > condition below. See > > > http://code.google.com/searchframe#OAMlx_jo-ck/src/media/media.gyp&exact_pack.... > > I might have been doing something wrong, but to use the use_ffmpeg variable I > had to define it in a variables section at the top of this file instead of down > here. Otherwise it was undefined (can't use a variable defined in a conditional > in the same conditional in which it was defined?). It's the same place that > media.gyp has it. Looks like the code I linked to, so LG.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tomfinegan@chromium.org/10899021/92049
Change committed as 163501 |
