Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11316045:
Add a libvpx video decoder to ClearKeyCdm and move the fake video decoder to its own class. (Closed)
|
Created:
8 years, 1 month ago by Tom Finegan Modified:
8 years ago Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd a libvpx video decoder to ClearKeyCdm and move the fake video decoder to its own class.
Add libvpx support behind the GYP flag use_libvpx.
Add GYP flag for control of fake video decoder support.
Add CdmVideoDecoder interface class, and use it for interaction with
FakeCdmVideoDecoder, FFmpegCdmVideoDecoder, and LibvpxVideoDecoder in
ClearKeyCdm.
Remove video decoder ifdefs from ClearKeyCdm, and handle the gritty
details of decoder selection and initialization in
cdm_video_decoder.
TEST=EME video decoding via ExternalClearKey continues to work.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=171183
Patch Set 1 #
Total comments: 3
Patch Set 2 : Clean up, and deal with some of the preprocessor evil in ClearKeyCdm. #
Total comments: 40
Patch Set 3 : Address comments. #
Total comments: 11
Patch Set 4 : Address comments. #
Total comments: 9
Patch Set 5 : Address comments on addressed comments... #Patch Set 6 : Rebase, fix incorrect DCHECK and fix include order. #Messages
Total messages: 19 (0 generated)
Behold, an ffmpeg video decoder class that also uses libvpx. Tis a foul thing, but it works on my machine. This needs some clean up, and I've been up to no good with the preprocessor. It'll serve as a starting point if we want to keep libvpx support around permanently. https://codereview.chromium.org/11316045/diff/1/webkit/media/crypto/ppapi/ffm... File webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.cc (right): https://codereview.chromium.org/11316045/diff/1/webkit/media/crypto/ppapi/ffm... webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.cc:570: Oops... I'll fix this whitespace.
Great! Thanks a lot! We'll use this to test the performance. At the same time, do you want us to review this patch? Or you'll clean it up a little bit more? http://codereview.chromium.org/11316045/diff/1/webkit/media/crypto/ppapi/ffmp... File webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.cc (right): http://codereview.chromium.org/11316045/diff/1/webkit/media/crypto/ppapi/ffmp... webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.cc:37: // against does, so maybe it's of some use... Is this what you are seeing? http://code.google.com/p/chromium/issues/detail?id=160622
https://codereview.chromium.org/11316045/diff/1/webkit/media/crypto/ppapi/ffm... File webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.cc (right): https://codereview.chromium.org/11316045/diff/1/webkit/media/crypto/ppapi/ffm... webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.cc:37: // against does, so maybe it's of some use... On 2012/11/16 17:33:02, xhwang wrote: > Is this what you are seeing? > http://code.google.com/p/chromium/issues/detail?id=160622 Yeah, that's the problem. Things look fine with --disable-accelerated-compositing added to the command line. Thanks!
Cleaned up, and took a stab at fixing some of the ifdef hell in ClearKeyCdm (by hiding it elsewhere). PTAL, I think this is ready for review. Thanks! https://codereview.chromium.org/11316045/diff/9001/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/libvpx_cdm_video_decoder.cc (right): https://codereview.chromium.org/11316045/diff/9001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/libvpx_cdm_video_decoder.cc:21: // #define USE_COPYPLANE_WITH_LIBVPX 1 Should I get rid of this?
Thanks. General comments on structure and filenames (before we start reviewing files whose name may change). Let me know what you think. https://codereview.chromium.org/11316045/diff/9001/webkit/media/webkit_media.... File webkit/media/webkit_media.gypi (right): https://codereview.chromium.org/11316045/diff/9001/webkit/media/webkit_media.... webkit/media/webkit_media.gypi:1: # Copyright (c) 2012 The Chromium Authors. All rights reserved. General comment: We should consider following the platform-specific implementation pattern, which is a file that implements a generic class (CdmVideoDecoder) in a specific way. We don't actually need subclasses because a given binary will only use one. If we do this, I think we should also follow the file naming convention of cdm_video_decoder_{ffmpeg|libvpx|fake}.cc. See src/crypto/ for numerous examples (_mac, _win, _nss). https://codereview.chromium.org/11316045/diff/9001/webkit/media/webkit_media.... webkit/media/webkit_media.gypi:121: ['use_fake_video_decoder == 1' , { What about the fake audio decoder? https://codereview.chromium.org/11316045/diff/9001/webkit/media/webkit_media.... webkit/media/webkit_media.gypi:122: 'defines': ['CLEAR_KEY_CDM_USE_FAKE_VIDEO_DECODER'], Should we even need all these defines if we're just replacing source files. It seems we might need one for FFmpeg init but other than that not. https://codereview.chromium.org/11316045/diff/9001/webkit/media/webkit_media.... webkit/media/webkit_media.gypi:133: 'sources': [ These should probably be split up into separate conditionals that check that fake and libvpx are not being used.
Discussion in reply to comments... https://codereview.chromium.org/11316045/diff/9001/webkit/media/webkit_media.... File webkit/media/webkit_media.gypi (right): https://codereview.chromium.org/11316045/diff/9001/webkit/media/webkit_media.... webkit/media/webkit_media.gypi:1: # Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2012/11/17 00:09:09, ddorwin wrote: > General comment: We should consider following the platform-specific > implementation pattern, which is a file that implements a generic class > (CdmVideoDecoder) in a specific way. We don't actually need subclasses because a > given binary will only use one. > > If we do this, I think we should also follow the file naming convention of > cdm_video_decoder_{ffmpeg|libvpx|fake}.cc. > > See src/crypto/ for numerous examples (_mac, _win, _nss). I did things this way because I thought we would still want H.264 to work when libvpx was enabled. Should I just make ClearKeyCdm VP8 only when libvpx is enabled? If so, then going through with what you suggest makes sense. https://codereview.chromium.org/11316045/diff/9001/webkit/media/webkit_media.... webkit/media/webkit_media.gypi:121: ['use_fake_video_decoder == 1' , { On 2012/11/17 00:09:09, ddorwin wrote: > What about the fake audio decoder? I was going to do that in another CL. I thought this one was getting large enough already. :) https://codereview.chromium.org/11316045/diff/9001/webkit/media/webkit_media.... webkit/media/webkit_media.gypi:122: 'defines': ['CLEAR_KEY_CDM_USE_FAKE_VIDEO_DECODER'], On 2012/11/17 00:09:09, ddorwin wrote: > Should we even need all these defines if we're just replacing source files. It > seems we might need one for FFmpeg init but other than that not. Probably not if we do source file replacement. Let me know what you think about my response to your first comment in this file. https://codereview.chromium.org/11316045/diff/9001/webkit/media/webkit_media.... webkit/media/webkit_media.gypi:133: 'sources': [ On 2012/11/17 00:09:09, ddorwin wrote: > These should probably be split up into separate conditionals that check that > fake and libvpx are not being used. Ditto re what you think on response to your first comment. How this would be split up (if at all) depends on that... Also, eventually this needs to be separated into audio and video pieces so fake video and be used w/FFmpeg and vice versa.
I did not review libvpx_cdm_video_decoder.cc in depth. https://codereview.chromium.org/11316045/diff/9001/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/cdm_video_decoder.h (right): https://codereview.chromium.org/11316045/diff/9001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/cdm_video_decoder.h:9: #include "base/compiler_specific.h" Why is this needed? https://codereview.chromium.org/11316045/diff/9001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/cdm_video_decoder.h:13: namespace webkit_media { Should this be in a cdm namespace instead? It's not really WK. https://codereview.chromium.org/11316045/diff/9001/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/cdm_video_decoder_initializer.cc (right): https://codereview.chromium.org/11316045/diff/9001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/cdm_video_decoder_initializer.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. It's odd to have a .cc without a matching header. Should we drop "_initializer"? https://codereview.chromium.org/11316045/diff/9001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/cdm_video_decoder_initializer.cc:6: #include "base/compiler_specific.h" Why is this needed? https://codereview.chromium.org/11316045/diff/9001/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/clear_key_cdm.cc (right): https://codereview.chromium.org/11316045/diff/9001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/clear_key_cdm.cc:360: webkit_media::InitializeVideoDecoder(allocator_, video_decoder_config); s/Initialize/Create https://codereview.chromium.org/11316045/diff/9001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/clear_key_cdm.cc:364: Should we check that the codec is the same and either reinitialize or error out? Or assert as long as Chrome media does not support switching. https://codereview.chromium.org/11316045/diff/9001/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/libvpx_cdm_video_decoder.cc (right): https://codereview.chromium.org/11316045/diff/9001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/libvpx_cdm_video_decoder.cc:15: #define VPX_CODEC_DISABLE_COMPAT 1 Odd to have this inside the extern statement. https://codereview.chromium.org/11316045/diff/9001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/libvpx_cdm_video_decoder.cc:19: // Enable USE_COPYPLANE_WITH_LIBVPX to use |CopyPlane()| instead of memcpy to Is this faster? Why would we want to do one vs. the other? https://codereview.chromium.org/11316045/diff/9001/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/libvpx_cdm_video_decoder.h (right): https://codereview.chromium.org/11316045/diff/9001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/libvpx_cdm_video_decoder.h:13: typedef struct vpx_codec_ctx vpx_codec_ctx_t; Why? There should at least be a comment. https://codereview.chromium.org/11316045/diff/9001/webkit/media/webkit_media.... File webkit/media/webkit_media.gypi (right): https://codereview.chromium.org/11316045/diff/9001/webkit/media/webkit_media.... webkit/media/webkit_media.gypi:1: # Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2012/11/17 00:25:46, Tom Finegan wrote: > On 2012/11/17 00:09:09, ddorwin wrote: > > General comment: We should consider following the platform-specific > > implementation pattern, which is a file that implements a generic class > > (CdmVideoDecoder) in a specific way. We don't actually need subclasses because > a > > given binary will only use one. > > > > If we do this, I think we should also follow the file naming convention of > > cdm_video_decoder_{ffmpeg|libvpx|fake}.cc. > > > > See src/crypto/ for numerous examples (_mac, _win, _nss). > > I did things this way because I thought we would still want H.264 to work when > libvpx was enabled. Should I just make ClearKeyCdm VP8 only when libvpx is > enabled? If so, then going through with what you suggest makes sense. Oh, I forgot about that. :( https://codereview.chromium.org/11316045/diff/9001/webkit/media/webkit_media.... webkit/media/webkit_media.gypi:121: ['use_fake_video_decoder == 1' , { On 2012/11/17 00:25:46, Tom Finegan wrote: > On 2012/11/17 00:09:09, ddorwin wrote: > > What about the fake audio decoder? > > I was going to do that in another CL. I thought this one was getting large > enough already. :) SG https://codereview.chromium.org/11316045/diff/9001/webkit/media/webkit_media.... webkit/media/webkit_media.gypi:133: 'sources': [ On 2012/11/17 00:25:46, Tom Finegan wrote: > On 2012/11/17 00:09:09, ddorwin wrote: > > These should probably be split up into separate conditionals that check that > > fake and libvpx are not being used. > > Ditto re what you think on response to your first comment. How this would be > split up (if at all) depends on that... > > Also, eventually this needs to be separated into audio and video pieces so fake > video and be used w/FFmpeg and vice versa. As below, the video_decoder files should only be added if use_ffmpeg == 1 && use_fake_video_decoder == 0 https://codereview.chromium.org/11316045/diff/9001/webkit/media/webkit_media.... webkit/media/webkit_media.gypi:140: ['use_libvpx == 1' , { && use_fake_video_decoder == 0
PTAL, thanks! https://codereview.chromium.org/11316045/diff/9001/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/cdm_video_decoder.h (right): https://codereview.chromium.org/11316045/diff/9001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/cdm_video_decoder.h:9: #include "base/compiler_specific.h" On 2012/11/17 02:56:22, ddorwin wrote: > Why is this needed? Done. https://codereview.chromium.org/11316045/diff/9001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/cdm_video_decoder.h:13: namespace webkit_media { On 2012/11/17 02:56:22, ddorwin wrote: > Should this be in a cdm namespace instead? It's not really WK. All of the decoder wrappers are in webkit_media. I think all would need to be changed if we're changing the base class namespace. If we're going to do that, I would prefer to leave it for another CL. SG? https://codereview.chromium.org/11316045/diff/9001/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/cdm_video_decoder_initializer.cc (right): https://codereview.chromium.org/11316045/diff/9001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/cdm_video_decoder_initializer.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2012/11/17 02:56:22, ddorwin wrote: > It's odd to have a .cc without a matching header. Should we drop "_initializer"? Done. https://codereview.chromium.org/11316045/diff/9001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/cdm_video_decoder_initializer.cc:6: #include "base/compiler_specific.h" On 2012/11/17 02:56:22, ddorwin wrote: > Why is this needed? Done. https://codereview.chromium.org/11316045/diff/9001/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/clear_key_cdm.cc (right): https://codereview.chromium.org/11316045/diff/9001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/clear_key_cdm.cc:360: webkit_media::InitializeVideoDecoder(allocator_, video_decoder_config); On 2012/11/17 02:56:22, ddorwin wrote: > s/Initialize/Create Done. https://codereview.chromium.org/11316045/diff/9001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/clear_key_cdm.cc:364: On 2012/11/17 02:56:22, ddorwin wrote: > Should we check that the codec is the same and either reinitialize or error out? > Or assert as long as Chrome media does not support switching. I don't think it's necessary. The decoders all clean up after themselves in their destructors. We don't currently have a way to check the current codec used by the decoders. I could add that, or an is_initialized() method (this has the same problem though; which codec was I using before the Init() call?)... https://codereview.chromium.org/11316045/diff/9001/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/libvpx_cdm_video_decoder.cc (right): https://codereview.chromium.org/11316045/diff/9001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/libvpx_cdm_video_decoder.cc:15: #define VPX_CODEC_DISABLE_COMPAT 1 On 2012/11/17 02:56:22, ddorwin wrote: > Odd to have this inside the extern statement. Moved outside. https://codereview.chromium.org/11316045/diff/9001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/libvpx_cdm_video_decoder.cc:19: // Enable USE_COPYPLANE_WITH_LIBVPX to use |CopyPlane()| instead of memcpy to On 2012/11/17 02:56:22, ddorwin wrote: > Is this faster? Why would we want to do one vs. the other? Discussed offline. Using memcpy to copy YUV frame buffer planes from libvpx output buffers to cdm::VideoFrame buffers allocates and copies more data than is strictly necessary (because each line in the target buffer is bytes_per_row long instead of display_width_bytes long). Not sure which is faster: we'll have to test it. https://codereview.chromium.org/11316045/diff/9001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/libvpx_cdm_video_decoder.cc:21: // #define USE_COPYPLANE_WITH_LIBVPX 1 On 2012/11/16 23:56:11, Tom Finegan wrote: > Should I get rid of this? Leaving until above perf question is resolved. https://codereview.chromium.org/11316045/diff/9001/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/libvpx_cdm_video_decoder.h (right): https://codereview.chromium.org/11316045/diff/9001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/libvpx_cdm_video_decoder.h:13: typedef struct vpx_codec_ctx vpx_codec_ctx_t; On 2012/11/17 02:56:22, ddorwin wrote: > Why? > There should at least be a comment. Switched weird looking duplicate typedefs to normal forward declarations. https://codereview.chromium.org/11316045/diff/9001/webkit/media/webkit_media.... File webkit/media/webkit_media.gypi (right): https://codereview.chromium.org/11316045/diff/9001/webkit/media/webkit_media.... webkit/media/webkit_media.gypi:133: 'sources': [ On 2012/11/17 02:56:22, ddorwin wrote: > On 2012/11/17 00:25:46, Tom Finegan wrote: > > On 2012/11/17 00:09:09, ddorwin wrote: > > > These should probably be split up into separate conditionals that check that > > > fake and libvpx are not being used. > > > > Ditto re what you think on response to your first comment. How this would be > > split up (if at all) depends on that... > > > > Also, eventually this needs to be separated into audio and video pieces so > fake > > video and be used w/FFmpeg and vice versa. > > As below, the video_decoder files should only be added if use_ffmpeg == 1 && > use_fake_video_decoder == 0 Done. https://codereview.chromium.org/11316045/diff/9001/webkit/media/webkit_media.... webkit/media/webkit_media.gypi:140: ['use_libvpx == 1' , { On 2012/11/17 02:56:22, ddorwin wrote: > && use_fake_video_decoder == 0 Done.
Didn't review the whole CL. Have an idea to use getenv() instead of #if defined() for discussion. http://codereview.chromium.org/11316045/diff/4002/webkit/media/crypto/ppapi/c... File webkit/media/crypto/ppapi/cdm_video_decoder.cc (right): http://codereview.chromium.org/11316045/diff/4002/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/cdm_video_decoder.cc:28: #if defined(CLEAR_KEY_CDM_USE_FAKE_VIDEO_DECODER) I wonder if we can use getenv() to make this a runtime option. For example, if CLEAR_KEY_CDM_USE_FAKE_VIDEO_DECODER is defined in environment variable, then use the fake decoder. If CLEAR_KEY_CDM_USE_LIBVPX_DECODER is defined, then use vpx. By default, we use ffmpeg. This way, we can avoid making changes to the gyp file, avoid the #if/#else mess here, and we don't need to rebuild the CDM if I want to test something else. WDYT? http://codereview.chromium.org/11316045/diff/4002/webkit/media/crypto/ppapi/f... File webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.h (right): http://codereview.chromium.org/11316045/diff/4002/webkit/media/crypto/ppapi/f... webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.h:35: const cdm::Size& data_size); move static method up to line 20?
PTAL, comments on xhwang's comments, and needs LGTMs if things are ok as they are. https://codereview.chromium.org/11316045/diff/4002/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/cdm_video_decoder.cc (right): https://codereview.chromium.org/11316045/diff/4002/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/cdm_video_decoder.cc:28: #if defined(CLEAR_KEY_CDM_USE_FAKE_VIDEO_DECODER) On 2012/11/20 06:46:20, xhwang wrote: > I wonder if we can use getenv() to make this a runtime option. For example, if > CLEAR_KEY_CDM_USE_FAKE_VIDEO_DECODER is defined in environment variable, then > use the fake decoder. If CLEAR_KEY_CDM_USE_LIBVPX_DECODER is defined, then use > vpx. By default, we use ffmpeg. This way, we can avoid making changes to the gyp > file, avoid the #if/#else mess here, and we don't need to rebuild the CDM if I > want to test something else. WDYT? The only problem I see with doing it the way you suggest is that we'll always be linking in all of the decoders instead of only those we need. The CDM will be bigger... Anyway, I like the suggestion, but I don't think we should make the change in this CL. I would prefer to get this landed, clean up the fake audio decoder stuff, and then do the getenv() stuff. What do you guys think? https://codereview.chromium.org/11316045/diff/4002/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.h (right): https://codereview.chromium.org/11316045/diff/4002/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.h:35: const cdm::Size& data_size); On 2012/11/20 06:46:20, xhwang wrote: > move static method up to line 20? Is this a style guide thing that I'm unaware of? I just want to understand the reasoning behind changing the order. Your comment also applies to ffmpeg_cdm_video_decoder.h, btw.
Response to the comments only. I'll review the rest of the CL later. https://codereview.chromium.org/11316045/diff/4002/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/cdm_video_decoder.cc (right): https://codereview.chromium.org/11316045/diff/4002/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/cdm_video_decoder.cc:28: #if defined(CLEAR_KEY_CDM_USE_FAKE_VIDEO_DECODER) On 2012/12/03 23:48:55, Tom Finegan wrote: > On 2012/11/20 06:46:20, xhwang wrote: > > I wonder if we can use getenv() to make this a runtime option. For example, if > > CLEAR_KEY_CDM_USE_FAKE_VIDEO_DECODER is defined in environment variable, then > > use the fake decoder. If CLEAR_KEY_CDM_USE_LIBVPX_DECODER is defined, then use > > vpx. By default, we use ffmpeg. This way, we can avoid making changes to the > gyp > > file, avoid the #if/#else mess here, and we don't need to rebuild the CDM if I > > want to test something else. WDYT? > > The only problem I see with doing it the way you suggest is that we'll always be > linking in all of the decoders instead of only those we need. The CDM will be > bigger... > > Anyway, I like the suggestion, but I don't think we should make the change in > this CL. I would prefer to get this landed, clean up the fake audio decoder > stuff, and then do the getenv() stuff. > > What do you guys think? I don't think it an issue to linkin all decoders for a test plugin. But I'll leave this discussion to ddorwin :) https://codereview.chromium.org/11316045/diff/4002/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.h (right): https://codereview.chromium.org/11316045/diff/4002/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.h:35: const cdm::Size& data_size); On 2012/12/03 23:48:55, Tom Finegan wrote: > On 2012/11/20 06:46:20, xhwang wrote: > > move static method up to line 20? > > Is this a style guide thing that I'm unaware of? > > I just want to understand the reasoning behind changing the order. Your comment > also applies to ffmpeg_cdm_video_decoder.h, btw. That's my mis-interpretation of the style guide. I just found recently what I was wrong. Please ignore my comment.
Almost there. Comments in PS2 and PS3. https://codereview.chromium.org/11316045/diff/9001/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/cdm_video_decoder.h (right): https://codereview.chromium.org/11316045/diff/9001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/cdm_video_decoder.h:13: namespace webkit_media { On 2012/11/17 04:35:06, Tom Finegan wrote: > On 2012/11/17 02:56:22, ddorwin wrote: > > Should this be in a cdm namespace instead? It's not really WK. > > All of the decoder wrappers are in webkit_media. I think all would need to be > changed if we're changing the base class namespace. If we're going to do that, I > would prefer to leave it for another CL. SG? SG. Please add a TODO or story. Either is fine. https://codereview.chromium.org/11316045/diff/9001/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/clear_key_cdm.cc (right): https://codereview.chromium.org/11316045/diff/9001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/clear_key_cdm.cc:364: On 2012/11/17 04:35:06, Tom Finegan wrote: > On 2012/11/17 02:56:22, ddorwin wrote: > > Should we check that the codec is the same and either reinitialize or error > out? > > Or assert as long as Chrome media does not support switching. > > I don't think it's necessary. The decoders all clean up after themselves in > their destructors. We don't currently have a way to check the current codec used > by the decoders. I could add that, or an is_initialized() method (this has the > same problem though; which codec was I using before the Init() call?)... This code allows repated calls without de-initializing. If we call with config A then with config B, it will try to use the video_decoder_ for potentially the wrong config. Where will that fail? It seems odd that we just totally ignore the config when reusing the video_decoder_. https://codereview.chromium.org/11316045/diff/9001/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/libvpx_cdm_video_decoder.cc (right): https://codereview.chromium.org/11316045/diff/9001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/libvpx_cdm_video_decoder.cc:19: // Enable USE_COPYPLANE_WITH_LIBVPX to use |CopyPlane()| instead of memcpy to On 2012/11/17 04:35:06, Tom Finegan wrote: > On 2012/11/17 02:56:22, ddorwin wrote: > > Is this faster? Why would we want to do one vs. the other? > > Discussed offline. Using memcpy to copy YUV frame buffer planes from libvpx > output buffers to cdm::VideoFrame buffers allocates and copies more data than is > strictly necessary (because each line in the target buffer is bytes_per_row long > instead of display_width_bytes long). Not sure which is faster: we'll have to > test it. Please add story. https://codereview.chromium.org/11316045/diff/4002/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/cdm_video_decoder.cc (right): https://codereview.chromium.org/11316045/diff/4002/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/cdm_video_decoder.cc:28: #if defined(CLEAR_KEY_CDM_USE_FAKE_VIDEO_DECODER) On 2012/12/04 01:31:03, xhwang wrote: > On 2012/12/03 23:48:55, Tom Finegan wrote: > > On 2012/11/20 06:46:20, xhwang wrote: > > > I wonder if we can use getenv() to make this a runtime option. For example, > if > > > CLEAR_KEY_CDM_USE_FAKE_VIDEO_DECODER is defined in environment variable, > then > > > use the fake decoder. If CLEAR_KEY_CDM_USE_LIBVPX_DECODER is defined, then > use > > > vpx. By default, we use ffmpeg. This way, we can avoid making changes to the > > gyp > > > file, avoid the #if/#else mess here, and we don't need to rebuild the CDM if > I > > > want to test something else. WDYT? > > > > The only problem I see with doing it the way you suggest is that we'll always > be > > linking in all of the decoders instead of only those we need. The CDM will be > > bigger... > > > > Anyway, I like the suggestion, but I don't think we should make the change in > > this CL. I would prefer to get this landed, clean up the fake audio decoder > > stuff, and then do the getenv() stuff. > > > > What do you guys think? > > I don't think it an issue to linkin all decoders for a test plugin. But I'll > leave this discussion to ddorwin :) Right, we're not worried about size since we never ship this (and the impact on build time should be minimal since we already build these decoders). I agree that we should hold off since we're pretty much at LG with this CL and the suggestion is an optimization. Caveats: * Can we pass random environment variables that are accessible within the sandbox? If not, it's not an option as proposed, but maybe there is something we can do. * We'd need to use base/ for obtaining the environment variables across platforms. * Would the code actually be considerably simpler? Would we end up with if statements instead of ifdefs? I agree that ifdefs are really hard to read, though. https://codereview.chromium.org/11316045/diff/4002/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/libvpx_cdm_video_decoder.cc (right): https://codereview.chromium.org/11316045/diff/4002/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/libvpx_cdm_video_decoder.cc:14: #define VPX_CODEC_DISABLE_COMPAT 1 Is this for the NEON issue (it's been a long time). We have a workaround, so we don't need this anymore. https://codereview.chromium.org/11316045/diff/4002/webkit/media/webkit_media.... File webkit/media/webkit_media.gypi (right): https://codereview.chromium.org/11316045/diff/4002/webkit/media/webkit_media.... webkit/media/webkit_media.gypi:128: ['use_ffmpeg == 1 and use_fake_video_decoder == 0' , { 'and use_fake_video_decoder == 0' only applies to ffmpeg_cdm_video_decoder.*.
PTAL... I sense another round of edits coming. :( https://codereview.chromium.org/11316045/diff/9001/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/cdm_video_decoder.h (right): https://codereview.chromium.org/11316045/diff/9001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/cdm_video_decoder.h:13: namespace webkit_media { On 2012/12/04 04:22:37, ddorwin wrote: > On 2012/11/17 04:35:06, Tom Finegan wrote: > > On 2012/11/17 02:56:22, ddorwin wrote: > > > Should this be in a cdm namespace instead? It's not really WK. > > > > All of the decoder wrappers are in webkit_media. I think all would need to be > > changed if we're changing the base class namespace. If we're going to do that, > I > > would prefer to leave it for another CL. SG? > > SG. Please add a TODO or story. Either is fine. Done. (added story) https://codereview.chromium.org/11316045/diff/9001/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/clear_key_cdm.cc (right): https://codereview.chromium.org/11316045/diff/9001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/clear_key_cdm.cc:364: On 2012/12/04 04:22:37, ddorwin wrote: > On 2012/11/17 04:35:06, Tom Finegan wrote: > > On 2012/11/17 02:56:22, ddorwin wrote: > > > Should we check that the codec is the same and either reinitialize or error > > out? > > > Or assert as long as Chrome media does not support switching. > > > > I don't think it's necessary. The decoders all clean up after themselves in > > their destructors. We don't currently have a way to check the current codec > used > > by the decoders. I could add that, or an is_initialized() method (this has the > > same problem though; which codec was I using before the Init() call?)... > > This code allows repated calls without de-initializing. If we call with config A > then with config B, it will try to use the video_decoder_ for potentially the > wrong config. Where will that fail? It seems odd that we just totally ignore the > config when reusing the video_decoder_. Yeah... that could be bad. Changed things so: - Initialize() will always create a new video decoder, and - calling Initialize() with an already initialized decoder will always result in a kSessionError. Is the latter part too heavy handed? Forcing callers to call Deinitalize() before additional calls to Initialize() seems like a good idea. https://codereview.chromium.org/11316045/diff/9001/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/libvpx_cdm_video_decoder.cc (right): https://codereview.chromium.org/11316045/diff/9001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/libvpx_cdm_video_decoder.cc:19: // Enable USE_COPYPLANE_WITH_LIBVPX to use |CopyPlane()| instead of memcpy to On 2012/12/04 04:22:37, ddorwin wrote: > On 2012/11/17 04:35:06, Tom Finegan wrote: > > On 2012/11/17 02:56:22, ddorwin wrote: > > > Is this faster? Why would we want to do one vs. the other? > > > > Discussed offline. Using memcpy to copy YUV frame buffer planes from libvpx > > output buffers to cdm::VideoFrame buffers allocates and copies more data than > is > > strictly necessary (because each line in the target buffer is bytes_per_row > long > > instead of display_width_bytes long). Not sure which is faster: we'll have to > > test it. > > Please add story. Done. https://codereview.chromium.org/11316045/diff/4002/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/libvpx_cdm_video_decoder.cc (right): https://codereview.chromium.org/11316045/diff/4002/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/libvpx_cdm_video_decoder.cc:14: #define VPX_CODEC_DISABLE_COMPAT 1 On 2012/12/04 04:22:37, ddorwin wrote: > Is this for the NEON issue (it's been a long time). We have a workaround, so we > don't need this anymore. This turns off the definition of some duplicated preprocessor symbols that predate the current names used by the libvpx API. For example, without it there's an extra I420 constant defined: #define IMG_FMT_I420 VPX_IMG_FMT_I420 There's other stuff too; that's just one example. https://codereview.chromium.org/11316045/diff/4002/webkit/media/webkit_media.... File webkit/media/webkit_media.gypi (right): https://codereview.chromium.org/11316045/diff/4002/webkit/media/webkit_media.... webkit/media/webkit_media.gypi:128: ['use_ffmpeg == 1 and use_fake_video_decoder == 0' , { On 2012/12/04 04:22:37, ddorwin wrote: > 'and use_fake_video_decoder == 0' only applies to ffmpeg_cdm_video_decoder.*. Yeah... this section is a mess now. I addressed your comment, and added a TODO.
Very close! Question for xhwang. https://codereview.chromium.org/11316045/diff/19001/webkit/media/crypto/ppapi... File webkit/media/crypto/ppapi/clear_key_cdm.cc (right): https://codereview.chromium.org/11316045/diff/19001/webkit/media/crypto/ppapi... webkit/media/crypto/ppapi/clear_key_cdm.cc:358: if (video_decoder_ && video_decoder_->is_initialized()) I'm not familiar with this logic. xhwang, is this what we want? https://codereview.chromium.org/11316045/diff/19001/webkit/media/crypto/ppapi... webkit/media/crypto/ppapi/clear_key_cdm.cc:361: video_decoder_ = CreateVideoDecoder(allocator_, video_decoder_config); // Any uninitialized decoder will be replaced. https://codereview.chromium.org/11316045/diff/19001/webkit/media/crypto/ppapi... File webkit/media/crypto/ppapi/libvpx_cdm_video_decoder.cc (right): https://codereview.chromium.org/11316045/diff/19001/webkit/media/crypto/ppapi... webkit/media/crypto/ppapi/libvpx_cdm_video_decoder.cc:14: #define VPX_CODEC_DISABLE_COMPAT 1 Please add a comment about why we set this. https://codereview.chromium.org/11316045/diff/19001/webkit/media/webkit_media... File webkit/media/webkit_media.gypi (right): https://codereview.chromium.org/11316045/diff/19001/webkit/media/webkit_media... webkit/media/webkit_media.gypi:142: 'defines': ['CLEAR_KEY_CDM_USE_FFMPEG_DECODER'], These 4 lines aren't necessary. Just the additional sources.
https://codereview.chromium.org/11316045/diff/19001/webkit/media/crypto/ppapi... File webkit/media/crypto/ppapi/clear_key_cdm.cc (right): https://codereview.chromium.org/11316045/diff/19001/webkit/media/crypto/ppapi... webkit/media/crypto/ppapi/clear_key_cdm.cc:358: if (video_decoder_ && video_decoder_->is_initialized()) On 2012/12/04 22:38:15, ddorwin wrote: > I'm not familiar with this logic. xhwang, is this what we want? Hmm, shouldn't we DCHECK(!video_decoder_->is_initialized()) ?
I can haz lgtm? https://codereview.chromium.org/11316045/diff/19001/webkit/media/crypto/ppapi... File webkit/media/crypto/ppapi/clear_key_cdm.cc (right): https://codereview.chromium.org/11316045/diff/19001/webkit/media/crypto/ppapi... webkit/media/crypto/ppapi/clear_key_cdm.cc:358: if (video_decoder_ && video_decoder_->is_initialized()) On 2012/12/04 23:07:46, xhwang wrote: > On 2012/12/04 22:38:15, ddorwin wrote: > > I'm not familiar with this logic. xhwang, is this what we want? > > Hmm, shouldn't we DCHECK(!video_decoder_->is_initialized()) ? Done. https://codereview.chromium.org/11316045/diff/19001/webkit/media/crypto/ppapi... webkit/media/crypto/ppapi/clear_key_cdm.cc:361: video_decoder_ = CreateVideoDecoder(allocator_, video_decoder_config); On 2012/12/04 22:38:15, ddorwin wrote: > // Any uninitialized decoder will be replaced. Done. https://codereview.chromium.org/11316045/diff/19001/webkit/media/crypto/ppapi... File webkit/media/crypto/ppapi/libvpx_cdm_video_decoder.cc (right): https://codereview.chromium.org/11316045/diff/19001/webkit/media/crypto/ppapi... webkit/media/crypto/ppapi/libvpx_cdm_video_decoder.cc:14: #define VPX_CODEC_DISABLE_COMPAT 1 On 2012/12/04 22:38:15, ddorwin wrote: > Please add a comment about why we set this. Done. https://codereview.chromium.org/11316045/diff/19001/webkit/media/webkit_media... File webkit/media/webkit_media.gypi (right): https://codereview.chromium.org/11316045/diff/19001/webkit/media/webkit_media... webkit/media/webkit_media.gypi:142: 'defines': ['CLEAR_KEY_CDM_USE_FFMPEG_DECODER'], On 2012/12/04 22:38:15, ddorwin wrote: > These 4 lines aren't necessary. Just the additional sources. Done.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tomfinegan@chromium.org/11316045/27001
Message was sent while issue was closed.
Change committed as 171183 |
