Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11242005:
Fake clear key CDM audio decoder. (Closed)
|
Created:
8 years, 2 months ago by xhwang Modified:
8 years, 1 month ago CC:
chromium-reviews, feature-media-reviews_chromium.org, darin-cc_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionFake clear key CDM audio decoder.
BUG=none
TEST=none
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=164092
Patch Set 1 #Patch Set 2 : #
Total comments: 6
Patch Set 3 : remove unused header #
Total comments: 9
Patch Set 4 : added more comments #Patch Set 5 : rebase #
Messages
Total messages: 10 (0 generated)
This is the fake audio decoder I use for testing. PTAL!
http://codereview.chromium.org/11242005/diff/2001/webkit/media/crypto/ppapi/c... File webkit/media/crypto/ppapi/clear_key_cdm.h (right): http://codereview.chromium.org/11242005/diff/2001/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/clear_key_cdm.h:15: #include "media/base/channel_layout.h" What's this for? http://codereview.chromium.org/11242005/diff/2001/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/clear_key_cdm.h:29: #if defined(CLEAR_KEY_CDM_USE_FAKE_VIDEO_DECODER) || defined (CLEAR_KEY_CDM_USE_FAKE_AUDIO_DECODER)?
http://codereview.chromium.org/11242005/diff/2001/webkit/media/crypto/ppapi/c... File webkit/media/crypto/ppapi/clear_key_cdm.h (right): http://codereview.chromium.org/11242005/diff/2001/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/clear_key_cdm.h:15: #include "media/base/channel_layout.h" On 2012/10/24 02:27:19, Tom Finegan wrote: > What's this for? Done. http://codereview.chromium.org/11242005/diff/2001/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/clear_key_cdm.h:29: #if defined(CLEAR_KEY_CDM_USE_FAKE_VIDEO_DECODER) On 2012/10/24 02:27:19, Tom Finegan wrote: > || defined (CLEAR_KEY_CDM_USE_FAKE_AUDIO_DECODER)? Hmm, for best testing coverage, I want to use the fake audio decoder w/ the FFmpeg video decoder and vice versa. Not sure what's the best way to do that though.
lgtm % comments. http://codereview.chromium.org/11242005/diff/6002/webkit/media/crypto/ppapi/c... File webkit/media/crypto/ppapi/clear_key_cdm.cc (right): http://codereview.chromium.org/11242005/diff/6002/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/clear_key_cdm.cc:380: DCHECK(last_timestamp_ != media::kNoTimestamp()); Will this DCHECK if you get EOS buffer in the first frame? http://codereview.chromium.org/11242005/diff/6002/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/clear_key_cdm.cc:396: last_timestamp_ = cur_timestamp; Should last_duration remain infinite? http://codereview.chromium.org/11242005/diff/6002/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/clear_key_cdm.cc:441: void ClearKeyCdm::GenerateFakeAudioFrames(cdm::AudioFrames* audio_frames) { // Generates one channel of audio.? http://codereview.chromium.org/11242005/diff/6002/webkit/media/crypto/ppapi/c... File webkit/media/crypto/ppapi/clear_key_cdm.h (right): http://codereview.chromium.org/11242005/diff/6002/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/clear_key_cdm.h:29: #undef CLEAR_KEY_CDM_USE_FFMPEG_DECODER Do we really want separate FFMPEG uses for video and audio? We're going to need them if we have the FAKEs separated. Maybe we can get fancy with #elif's in the .cc and && at line 28. We should try to keep it out of .gyp if possible I think.
lgtm http://codereview.chromium.org/11242005/diff/2001/webkit/media/crypto/ppapi/c... File webkit/media/crypto/ppapi/clear_key_cdm.h (right): http://codereview.chromium.org/11242005/diff/2001/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/clear_key_cdm.h:29: #if defined(CLEAR_KEY_CDM_USE_FAKE_VIDEO_DECODER) On 2012/10/24 16:40:39, xhwang wrote: > On 2012/10/24 02:27:19, Tom Finegan wrote: > > || defined (CLEAR_KEY_CDM_USE_FAKE_AUDIO_DECODER)? > > Hmm, for best testing coverage, I want to use the fake audio decoder w/ the > FFmpeg video decoder and vice versa. Not sure what's the best way to do that > though. I'll define CLEAR_KEY_CDM_USE_FFMPEG_AUDIO_DECODER CLEAR_KEY_CDM_USE_FFMPEG_VIDEO_DECODER in the gyp in my CL, and update this to handle the separate flags. If that ends up being too ugly we can discuss it more in http://codereview.chromium.org/11260007/
http://codereview.chromium.org/11242005/diff/2001/webkit/media/crypto/ppapi/c... File webkit/media/crypto/ppapi/clear_key_cdm.h (right): http://codereview.chromium.org/11242005/diff/2001/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/clear_key_cdm.h:29: #if defined(CLEAR_KEY_CDM_USE_FAKE_VIDEO_DECODER) On 2012/10/24 18:06:09, Tom Finegan wrote: > On 2012/10/24 16:40:39, xhwang wrote: > > On 2012/10/24 02:27:19, Tom Finegan wrote: > > > || defined (CLEAR_KEY_CDM_USE_FAKE_AUDIO_DECODER)? > > > > Hmm, for best testing coverage, I want to use the fake audio decoder w/ the > > FFmpeg video decoder and vice versa. Not sure what's the best way to do that > > though. > > I'll define CLEAR_KEY_CDM_USE_FFMPEG_AUDIO_DECODER > CLEAR_KEY_CDM_USE_FFMPEG_VIDEO_DECODER in the gyp in my CL, and update this to > handle the separate flags. > > If that ends up being too ugly we can discuss it more in > http://codereview.chromium.org/11260007/ Please don't. I want to avoid polluting the gyp unnecessarily. Let's land both of these then look at how we can fix this in a separate CL with less code. One of you please add a TODO to do so.
On 2012/10/24 18:09:31, ddorwin wrote: > http://codereview.chromium.org/11242005/diff/2001/webkit/media/crypto/ppapi/c... > File webkit/media/crypto/ppapi/clear_key_cdm.h (right): > > http://codereview.chromium.org/11242005/diff/2001/webkit/media/crypto/ppapi/c... > webkit/media/crypto/ppapi/clear_key_cdm.h:29: #if > defined(CLEAR_KEY_CDM_USE_FAKE_VIDEO_DECODER) > On 2012/10/24 18:06:09, Tom Finegan wrote: > > On 2012/10/24 16:40:39, xhwang wrote: > > > On 2012/10/24 02:27:19, Tom Finegan wrote: > > > > || defined (CLEAR_KEY_CDM_USE_FAKE_AUDIO_DECODER)? > > > > > > Hmm, for best testing coverage, I want to use the fake audio decoder w/ the > > > FFmpeg video decoder and vice versa. Not sure what's the best way to do that > > > though. > > > > I'll define CLEAR_KEY_CDM_USE_FFMPEG_AUDIO_DECODER > > CLEAR_KEY_CDM_USE_FFMPEG_VIDEO_DECODER in the gyp in my CL, and update this to > > handle the separate flags. > > > > If that ends up being too ugly we can discuss it more in > > http://codereview.chromium.org/11260007/ > > Please don't. I want to avoid polluting the gyp unnecessarily. Let's land both > of these then look at how we can fix this in a separate CL with less code. > > One of you please add a TODO to do so. I'll put the TODO in my CL. The only thing to note is that once my CL lands, assuming it remains as it is, enabling one fake decoder disables both audio and video from FFmpeg.
http://codereview.chromium.org/11242005/diff/6002/webkit/media/crypto/ppapi/c... File webkit/media/crypto/ppapi/clear_key_cdm.cc (right): http://codereview.chromium.org/11242005/diff/6002/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/clear_key_cdm.cc:380: DCHECK(last_timestamp_ != media::kNoTimestamp()); On 2012/10/24 17:59:27, ddorwin wrote: > Will this DCHECK if you get EOS buffer in the first frame? If the first frame is EOS, then last_duration_ shoud be media::kInfiniteDuration(). http://codereview.chromium.org/11242005/diff/6002/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/clear_key_cdm.cc:396: last_timestamp_ = cur_timestamp; On 2012/10/24 17:59:27, ddorwin wrote: > Should last_duration remain infinite? I would say yes. We don't have any valid duration now since we have only one timestamp. http://codereview.chromium.org/11242005/diff/6002/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/clear_key_cdm.cc:441: void ClearKeyCdm::GenerateFakeAudioFrames(cdm::AudioFrames* audio_frames) { On 2012/10/24 17:59:27, ddorwin wrote: > // Generates one channel of audio.? Not sure about "one channel of audio". But updated the comments :) http://codereview.chromium.org/11242005/diff/6002/webkit/media/crypto/ppapi/c... File webkit/media/crypto/ppapi/clear_key_cdm.h (right): http://codereview.chromium.org/11242005/diff/6002/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/clear_key_cdm.h:29: #undef CLEAR_KEY_CDM_USE_FFMPEG_DECODER On 2012/10/24 17:59:27, ddorwin wrote: > Do we really want separate FFMPEG uses for video and audio? We're going to need > them if we have the FAKEs separated. > Maybe we can get fancy with #elif's in the .cc and && at line 28. We should try > to keep it out of .gyp if possible I think. Do you want me to fix this here? Or in Tom's ffmpeg cmd audio decoder patch?
lgtm http://codereview.chromium.org/11242005/diff/6002/webkit/media/crypto/ppapi/c... File webkit/media/crypto/ppapi/clear_key_cdm.h (right): http://codereview.chromium.org/11242005/diff/6002/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/clear_key_cdm.h:29: #undef CLEAR_KEY_CDM_USE_FFMPEG_DECODER On 2012/10/24 19:45:57, xhwang wrote: > On 2012/10/24 17:59:27, ddorwin wrote: > > Do we really want separate FFMPEG uses for video and audio? We're going to > need > > them if we have the FAKEs separated. > > Maybe we can get fancy with #elif's in the .cc and && at line 28. We should > try > > to keep it out of .gyp if possible I think. > > Do you want me to fix this here? Or in Tom's ffmpeg cmd audio decoder patch? In patch 2, Tom said he will add a TODO, then we'll tackle independence in a different CL.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xhwang@chromium.org/11242005/18001 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
