Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11260007:
Add FFmpeg audio decoder for the clear key CDM. (Closed)
|
Created:
8 years, 2 months ago by Tom Finegan 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. |
DescriptionAdd FFmpeg audio decoder for the clear key CDM.
BUG=141780
TEST=
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=164162
Patch Set 1 #Patch Set 2 : Rebased on 11242005 and 11189082 #
Total comments: 32
Patch Set 3 : Address comments. #
Total comments: 2
Patch Set 4 : Rebased on master and 11242005. #
Total comments: 9
Patch Set 5 : Minor changes to make things work. #
Total comments: 8
Patch Set 6 : Address comments. #Patch Set 7 : Add TODO. #
Messages
Total messages: 17 (0 generated)
First pass at the audio decoder. Not tested yet. Note: This CL is based on xhwang's CLs: http://codereview.chromium.org/11189082/ http://codereview.chromium.org/11242005/ Those should land before this one does. To avoid duplicating reviews/reading your own code: - Save clear_key_cdm.cc and clear_key_cdm.h until the above land. - Review only ffmpeg_cdm_audio_decoder.* and webkit_media.gypi Questions: - Is what I'm doing for EoS going to work? Seems weird. Should I just send no frames back? - Do we want to be able to enable the FFmpeg audio and video decoders separately? (i.e. do we need CLEAR_KEY_CDM_USE_FFMPEG_AUDIO_DECODER and CLEAR_KEY_CDM_USE_FFMPEG_VIDEO_DECODER, or is CLEAR_KEY_CDM_USE_FFMPEG_DECODER good enough) PTAL, thanks!
- Is what I'm doing for EoS going to work? Seems weird. Should I just send no frames back? Commented in code. We should send kNeedMoreData in that case. - Do we want to be able to enable the FFmpeg audio and video decoders separately? (i.e. do we need CLEAR_KEY_CDM_USE_FFMPEG_AUDIO_DECODER and CLEAR_KEY_CDM_USE_FFMPEG_VIDEO_DECODER, or is CLEAR_KEY_CDM_USE_FFMPEG_DECODER good enough) I'd like to be able to enable them separately for testing purposes. But I do agree the corrent macro approach introduces too much noise. If we plan to add more decoders we definitely need to clean up the current implementation. http://codereview.chromium.org/11260007/diff/2001/webkit/media/crypto/ppapi/f... File webkit/media/crypto/ppapi/ffmpeg_cdm_audio_decoder.cc (right): http://codereview.chromium.org/11260007/diff/2001/webkit/media/crypto/ppapi/f... webkit/media/crypto/ppapi/ffmpeg_cdm_audio_decoder.cc:6: add #include <algorithm> for std::min http://codereview.chromium.org/11260007/diff/2001/webkit/media/crypto/ppapi/f... webkit/media/crypto/ppapi/ffmpeg_cdm_audio_decoder.cc:88: : allocator_(allocator), initialization order should match declaration order. http://codereview.chromium.org/11260007/diff/2001/webkit/media/crypto/ppapi/f... webkit/media/crypto/ppapi/ffmpeg_cdm_audio_decoder.cc:96: total_frames_decoded_(0), If we really need to use double for total_frames_decoded_, we should use 0.0 here. http://codereview.chromium.org/11260007/diff/2001/webkit/media/crypto/ppapi/f... webkit/media/crypto/ppapi/ffmpeg_cdm_audio_decoder.cc:119: ReleaseFFmpegResources(); In which case do we need this? Are we worrying that Initialize() is called without de-init? Shouldn't the check on line 113 cover this already? http://codereview.chromium.org/11260007/diff/2001/webkit/media/crypto/ppapi/f... webkit/media/crypto/ppapi/ffmpeg_cdm_audio_decoder.cc:139: is_initialized_ = true; Move this down to line 144. http://codereview.chromium.org/11260007/diff/2001/webkit/media/crypto/ppapi/f... webkit/media/crypto/ppapi/ffmpeg_cdm_audio_decoder.cc:165: config.channel_count > 0 && config.channel_count <= kMaxVorbisChannels && split this to two lines to be consistent with the rest. also indent to align with "config.codec == ... "? http://codereview.chromium.org/11260007/diff/2001/webkit/media/crypto/ppapi/f... webkit/media/crypto/ppapi/ffmpeg_cdm_audio_decoder.cc:236: break; The break here will hit line 315. In that case we are returning kSuccess with a null buffer. I think that'll hit a DCHECK on the way to media code. How about just return kDecodeError here? I don't really know the impact of this bug though. http://codereview.chromium.org/11260007/diff/2001/webkit/media/crypto/ppapi/f... webkit/media/crypto/ppapi/ffmpeg_cdm_audio_decoder.cc:254: const uint8* decoded_audio_data = NULL; we are usng uint8_t in other places in this file. http://codereview.chromium.org/11260007/diff/2001/webkit/media/crypto/ppapi/f... webkit/media/crypto/ppapi/ffmpeg_cdm_audio_decoder.cc:281: base::TimeDelta output_timestamp = GetNextOutputTimestamp(); hmm, ISTM that this timestamp is the time of the last decoded frame, not the first one. I know this is from media code. I'll double check tomorrow morning. http://codereview.chromium.org/11260007/diff/2001/webkit/media/crypto/ppapi/f... webkit/media/crypto/ppapi/ffmpeg_cdm_audio_decoder.cc:294: is_end_of_stream)) { I am overwhelmed by the logic in this function (I know it's from media code, so I am not complaining :)) Let's look at the logic here. IsEndOfOutputStream() can be true only when is_end_of_stream is true. But in this case, packet.size must be zero. So this do/while loop can only run once. Since we have no data, "result" must be zero here (negative result has triggered break on line 236). So what really matters is the decoded_audio_size. It must be zero here since we are at the "else if" for "if (decoded_audio_size > 0)". So the condition here is really equivalent to "else if (is_end_of_stream)". Anyway, in this case, we should return kNeedMoreData instead of serialize an end of stream buffer. (see below comment). http://codereview.chromium.org/11260007/diff/2001/webkit/media/crypto/ppapi/f... webkit/media/crypto/ppapi/ffmpeg_cdm_audio_decoder.cc:298: SerializeInt64(0); The new contract is: suppose when the input is EOS, we can still produce N frames. Then we return N frames with kSuccess as normal. Then in the next call (the input must be EOS again), we return kNeedMoreData. http://codereview.chromium.org/11260007/diff/2001/webkit/media/crypto/ppapi/f... webkit/media/crypto/ppapi/ffmpeg_cdm_audio_decoder.cc:318: void FFmpegCdmAudioDecoder::ResetOutputTime() { This function name is a little misleading as we are also setting last_input_timestamp_. http://codereview.chromium.org/11260007/diff/2001/webkit/media/crypto/ppapi/f... File webkit/media/crypto/ppapi/ffmpeg_cdm_audio_decoder.h (right): http://codereview.chromium.org/11260007/diff/2001/webkit/media/crypto/ppapi/f... webkit/media/crypto/ppapi/ffmpeg_cdm_audio_decoder.h:34: // Returns |cdm::kDecodeError| when decoding fails. // Decodes |compressed_buffer|. // Returns |cdm::kSuccess| and stores output frame in |decoded_frames| when an output is available. // Returns |cdm::kNeedMoreData| when |compressed_frame| does not produce a*n* output. // Returns |cdm::kDecodeError| when decoding fails. http://codereview.chromium.org/11260007/diff/2001/webkit/media/crypto/ppapi/f... webkit/media/crypto/ppapi/ffmpeg_cdm_audio_decoder.h:54: cdm::Allocator* const allocator_; Declaration order doesn't match initialization order. Can we move allocator and is_initialized up? http://codereview.chromium.org/11260007/diff/2001/webkit/media/crypto/ppapi/f... webkit/media/crypto/ppapi/ffmpeg_cdm_audio_decoder.h:63: double total_frames_decoded_; I know this is from media code, but it seems to me this number is always an integer. How about use an int? When we do division, we can always cast to double to preserve precision if needed.
http://codereview.chromium.org/11260007/diff/2001/webkit/media/crypto/ppapi/f... File webkit/media/crypto/ppapi/ffmpeg_cdm_audio_decoder.cc (right): http://codereview.chromium.org/11260007/diff/2001/webkit/media/crypto/ppapi/f... webkit/media/crypto/ppapi/ffmpeg_cdm_audio_decoder.cc:281: base::TimeDelta output_timestamp = GetNextOutputTimestamp(); On 2012/10/24 08:18:15, xhwang wrote: > hmm, ISTM that this timestamp is the time of the last decoded frame, not the > first one. I know this is from media code. I'll double check tomorrow morning. Okay, now I understand how it works ;) Could you please move line 291 to 282? These two lines are closely related.
I think everything is resolved, but perhaps I am smoking crack. PTAL, thanks! http://codereview.chromium.org/11260007/diff/2001/webkit/media/crypto/ppapi/f... File webkit/media/crypto/ppapi/ffmpeg_cdm_audio_decoder.cc (right): http://codereview.chromium.org/11260007/diff/2001/webkit/media/crypto/ppapi/f... webkit/media/crypto/ppapi/ffmpeg_cdm_audio_decoder.cc:6: On 2012/10/24 08:18:15, xhwang wrote: > add #include <algorithm> for std::min Done. http://codereview.chromium.org/11260007/diff/2001/webkit/media/crypto/ppapi/f... webkit/media/crypto/ppapi/ffmpeg_cdm_audio_decoder.cc:88: : allocator_(allocator), On 2012/10/24 08:18:15, xhwang wrote: > initialization order should match declaration order. Done. http://codereview.chromium.org/11260007/diff/2001/webkit/media/crypto/ppapi/f... webkit/media/crypto/ppapi/ffmpeg_cdm_audio_decoder.cc:96: total_frames_decoded_(0), On 2012/10/24 08:18:15, xhwang wrote: > If we really need to use double for total_frames_decoded_, we should use 0.0 > here. It's int64_t now. http://codereview.chromium.org/11260007/diff/2001/webkit/media/crypto/ppapi/f... webkit/media/crypto/ppapi/ffmpeg_cdm_audio_decoder.cc:119: ReleaseFFmpegResources(); On 2012/10/24 08:18:15, xhwang wrote: > In which case do we need this? Are we worrying that Initialize() is called > without de-init? Shouldn't the check on line 113 cover this already? Done, here and in the video decoder. Copied from media version. http://codereview.chromium.org/11260007/diff/2001/webkit/media/crypto/ppapi/f... webkit/media/crypto/ppapi/ffmpeg_cdm_audio_decoder.cc:139: is_initialized_ = true; On 2012/10/24 08:18:15, xhwang wrote: > Move this down to line 144. Done. http://codereview.chromium.org/11260007/diff/2001/webkit/media/crypto/ppapi/f... webkit/media/crypto/ppapi/ffmpeg_cdm_audio_decoder.cc:165: config.channel_count > 0 && config.channel_count <= kMaxVorbisChannels && On 2012/10/24 08:18:15, xhwang wrote: > split this to two lines to be consistent with the rest. also indent to align > with "config.codec == ... "? Done. http://codereview.chromium.org/11260007/diff/2001/webkit/media/crypto/ppapi/f... webkit/media/crypto/ppapi/ffmpeg_cdm_audio_decoder.cc:236: break; On 2012/10/24 08:18:15, xhwang wrote: > The break here will hit line 315. In that case we are returning kSuccess with a > null buffer. I think that'll hit a DCHECK on the way to media code. How about > just return kDecodeError here? I don't really know the impact of this bug > though. Done. http://codereview.chromium.org/11260007/diff/2001/webkit/media/crypto/ppapi/f... webkit/media/crypto/ppapi/ffmpeg_cdm_audio_decoder.cc:254: const uint8* decoded_audio_data = NULL; On 2012/10/24 08:18:15, xhwang wrote: > we are usng uint8_t in other places in this file. _t added; missed this one after the copy from media. http://codereview.chromium.org/11260007/diff/2001/webkit/media/crypto/ppapi/f... webkit/media/crypto/ppapi/ffmpeg_cdm_audio_decoder.cc:281: base::TimeDelta output_timestamp = GetNextOutputTimestamp(); On 2012/10/24 16:33:21, xhwang wrote: > On 2012/10/24 08:18:15, xhwang wrote: > > hmm, ISTM that this timestamp is the time of the last decoded frame, not the > > first one. I know this is from media code. I'll double check tomorrow morning. > > Okay, now I understand how it works ;) Could you please move line 291 to 282? > These two lines are closely related. Done. http://codereview.chromium.org/11260007/diff/2001/webkit/media/crypto/ppapi/f... webkit/media/crypto/ppapi/ffmpeg_cdm_audio_decoder.cc:294: is_end_of_stream)) { On 2012/10/24 08:18:15, xhwang wrote: > I am overwhelmed by the logic in this function (I know it's from media code, so > I am not complaining :)) > > Let's look at the logic here. IsEndOfOutputStream() can be true only when > is_end_of_stream is true. But in this case, packet.size must be zero. So this > do/while loop can only run once. Since we have no data, "result" must be zero > here (negative result has triggered break on line 236). So what really matters > is the decoded_audio_size. It must be zero here since we are at the "else if" > for "if (decoded_audio_size > 0)". So the condition here is really equivalent to > "else if (is_end_of_stream)". > > Anyway, in this case, we should return kNeedMoreData instead of serialize an end > of stream buffer. (see below comment). Done (returning kNeedMoreData). Changed nothing else. Should I change the conditions? http://codereview.chromium.org/11260007/diff/2001/webkit/media/crypto/ppapi/f... webkit/media/crypto/ppapi/ffmpeg_cdm_audio_decoder.cc:298: SerializeInt64(0); On 2012/10/24 08:18:15, xhwang wrote: > The new contract is: suppose when the input is EOS, we can still produce N > frames. Then we return N frames with kSuccess as normal. Then in the next call > (the input must be EOS again), we return kNeedMoreData. Done. http://codereview.chromium.org/11260007/diff/2001/webkit/media/crypto/ppapi/f... webkit/media/crypto/ppapi/ffmpeg_cdm_audio_decoder.cc:318: void FFmpegCdmAudioDecoder::ResetOutputTime() { On 2012/10/24 08:18:15, xhwang wrote: > This function name is a little misleading as we are also setting > last_input_timestamp_. Renamed to ResetAudioTimingData(). http://codereview.chromium.org/11260007/diff/2001/webkit/media/crypto/ppapi/f... File webkit/media/crypto/ppapi/ffmpeg_cdm_audio_decoder.h (right): http://codereview.chromium.org/11260007/diff/2001/webkit/media/crypto/ppapi/f... webkit/media/crypto/ppapi/ffmpeg_cdm_audio_decoder.h:34: // Returns |cdm::kDecodeError| when decoding fails. On 2012/10/24 08:18:15, xhwang wrote: > // Decodes |compressed_buffer|. > // Returns |cdm::kSuccess| and stores output frame in |decoded_frames| when an > output is available. > // Returns |cdm::kNeedMoreData| when |compressed_frame| does not produce a*n* > output. > // Returns |cdm::kDecodeError| when decoding fails. I didn't use your wording, but your main concern seems to be that the first thing the second second says is "Returns X". That's addressed, assuming you agree with the new comment. :) http://codereview.chromium.org/11260007/diff/2001/webkit/media/crypto/ppapi/f... webkit/media/crypto/ppapi/ffmpeg_cdm_audio_decoder.h:54: cdm::Allocator* const allocator_; On 2012/10/24 08:18:15, xhwang wrote: > Declaration order doesn't match initialization order. Can we move allocator > and is_initialized up? Done. http://codereview.chromium.org/11260007/diff/2001/webkit/media/crypto/ppapi/f... webkit/media/crypto/ppapi/ffmpeg_cdm_audio_decoder.h:63: double total_frames_decoded_; On 2012/10/24 08:18:15, xhwang wrote: > I know this is from media code, but it seems to me this number is always an > integer. How about use an int? When we do division, we can always cast to double > to preserve precision if needed. Done.
On 2012/10/24 21:16:44, Tom Finegan wrote: > I think everything is resolved, but perhaps I am smoking crack. PTAL, thanks! > Ok, smoking a little crack. The TODO about dealing with the ifdef hell isn't in here yet. > http://codereview.chromium.org/11260007/diff/2001/webkit/media/crypto/ppapi/f... > File webkit/media/crypto/ppapi/ffmpeg_cdm_audio_decoder.cc (right): > > http://codereview.chromium.org/11260007/diff/2001/webkit/media/crypto/ppapi/f... > webkit/media/crypto/ppapi/ffmpeg_cdm_audio_decoder.cc:6: > On 2012/10/24 08:18:15, xhwang wrote: > > add #include <algorithm> for std::min > > Done. > > http://codereview.chromium.org/11260007/diff/2001/webkit/media/crypto/ppapi/f... > webkit/media/crypto/ppapi/ffmpeg_cdm_audio_decoder.cc:88: : > allocator_(allocator), > On 2012/10/24 08:18:15, xhwang wrote: > > initialization order should match declaration order. > > Done. > > http://codereview.chromium.org/11260007/diff/2001/webkit/media/crypto/ppapi/f... > webkit/media/crypto/ppapi/ffmpeg_cdm_audio_decoder.cc:96: > total_frames_decoded_(0), > On 2012/10/24 08:18:15, xhwang wrote: > > If we really need to use double for total_frames_decoded_, we should use 0.0 > > here. > > It's int64_t now. > > http://codereview.chromium.org/11260007/diff/2001/webkit/media/crypto/ppapi/f... > webkit/media/crypto/ppapi/ffmpeg_cdm_audio_decoder.cc:119: > ReleaseFFmpegResources(); > On 2012/10/24 08:18:15, xhwang wrote: > > In which case do we need this? Are we worrying that Initialize() is called > > without de-init? Shouldn't the check on line 113 cover this already? > > Done, here and in the video decoder. Copied from media version. > > http://codereview.chromium.org/11260007/diff/2001/webkit/media/crypto/ppapi/f... > webkit/media/crypto/ppapi/ffmpeg_cdm_audio_decoder.cc:139: is_initialized_ = > true; > On 2012/10/24 08:18:15, xhwang wrote: > > Move this down to line 144. > > Done. > > http://codereview.chromium.org/11260007/diff/2001/webkit/media/crypto/ppapi/f... > webkit/media/crypto/ppapi/ffmpeg_cdm_audio_decoder.cc:165: config.channel_count > > 0 && config.channel_count <= kMaxVorbisChannels && > On 2012/10/24 08:18:15, xhwang wrote: > > split this to two lines to be consistent with the rest. also indent to align > > with "config.codec == ... "? > > Done. > > http://codereview.chromium.org/11260007/diff/2001/webkit/media/crypto/ppapi/f... > webkit/media/crypto/ppapi/ffmpeg_cdm_audio_decoder.cc:236: break; > On 2012/10/24 08:18:15, xhwang wrote: > > The break here will hit line 315. In that case we are returning kSuccess with > a > > null buffer. I think that'll hit a DCHECK on the way to media code. How about > > just return kDecodeError here? I don't really know the impact of this bug > > though. > > Done. > > http://codereview.chromium.org/11260007/diff/2001/webkit/media/crypto/ppapi/f... > webkit/media/crypto/ppapi/ffmpeg_cdm_audio_decoder.cc:254: const uint8* > decoded_audio_data = NULL; > On 2012/10/24 08:18:15, xhwang wrote: > > we are usng uint8_t in other places in this file. > > _t added; missed this one after the copy from media. > > http://codereview.chromium.org/11260007/diff/2001/webkit/media/crypto/ppapi/f... > webkit/media/crypto/ppapi/ffmpeg_cdm_audio_decoder.cc:281: base::TimeDelta > output_timestamp = GetNextOutputTimestamp(); > On 2012/10/24 16:33:21, xhwang wrote: > > On 2012/10/24 08:18:15, xhwang wrote: > > > hmm, ISTM that this timestamp is the time of the last decoded frame, not the > > > first one. I know this is from media code. I'll double check tomorrow > morning. > > > > Okay, now I understand how it works ;) Could you please move line 291 to 282? > > These two lines are closely related. > > Done. > > http://codereview.chromium.org/11260007/diff/2001/webkit/media/crypto/ppapi/f... > webkit/media/crypto/ppapi/ffmpeg_cdm_audio_decoder.cc:294: is_end_of_stream)) { > On 2012/10/24 08:18:15, xhwang wrote: > > I am overwhelmed by the logic in this function (I know it's from media code, > so > > I am not complaining :)) > > > > Let's look at the logic here. IsEndOfOutputStream() can be true only when > > is_end_of_stream is true. But in this case, packet.size must be zero. So this > > do/while loop can only run once. Since we have no data, "result" must be zero > > here (negative result has triggered break on line 236). So what really > matters > > is the decoded_audio_size. It must be zero here since we are at the "else if" > > for "if (decoded_audio_size > 0)". So the condition here is really equivalent > to > > "else if (is_end_of_stream)". > > > > Anyway, in this case, we should return kNeedMoreData instead of serialize an > end > > of stream buffer. (see below comment). > > Done (returning kNeedMoreData). Changed nothing else. Should I change the > conditions? > > http://codereview.chromium.org/11260007/diff/2001/webkit/media/crypto/ppapi/f... > webkit/media/crypto/ppapi/ffmpeg_cdm_audio_decoder.cc:298: SerializeInt64(0); > On 2012/10/24 08:18:15, xhwang wrote: > > The new contract is: suppose when the input is EOS, we can still produce N > > frames. Then we return N frames with kSuccess as normal. Then in the next call > > (the input must be EOS again), we return kNeedMoreData. > > Done. > > http://codereview.chromium.org/11260007/diff/2001/webkit/media/crypto/ppapi/f... > webkit/media/crypto/ppapi/ffmpeg_cdm_audio_decoder.cc:318: void > FFmpegCdmAudioDecoder::ResetOutputTime() { > On 2012/10/24 08:18:15, xhwang wrote: > > This function name is a little misleading as we are also setting > > last_input_timestamp_. > > Renamed to ResetAudioTimingData(). > > http://codereview.chromium.org/11260007/diff/2001/webkit/media/crypto/ppapi/f... > File webkit/media/crypto/ppapi/ffmpeg_cdm_audio_decoder.h (right): > > http://codereview.chromium.org/11260007/diff/2001/webkit/media/crypto/ppapi/f... > webkit/media/crypto/ppapi/ffmpeg_cdm_audio_decoder.h:34: // Returns > |cdm::kDecodeError| when decoding fails. > On 2012/10/24 08:18:15, xhwang wrote: > > // Decodes |compressed_buffer|. > > // Returns |cdm::kSuccess| and stores output frame in |decoded_frames| when an > > output is available. > > // Returns |cdm::kNeedMoreData| when |compressed_frame| does not produce a*n* > > output. > > // Returns |cdm::kDecodeError| when decoding fails. > > I didn't use your wording, but your main concern seems to be that the first > thing the second second says is "Returns X". That's addressed, assuming you > agree with the new comment. :) > > http://codereview.chromium.org/11260007/diff/2001/webkit/media/crypto/ppapi/f... > webkit/media/crypto/ppapi/ffmpeg_cdm_audio_decoder.h:54: cdm::Allocator* const > allocator_; > On 2012/10/24 08:18:15, xhwang wrote: > > Declaration order doesn't match initialization order. Can we move allocator > > and is_initialized up? > > Done. > > http://codereview.chromium.org/11260007/diff/2001/webkit/media/crypto/ppapi/f... > webkit/media/crypto/ppapi/ffmpeg_cdm_audio_decoder.h:63: double > total_frames_decoded_; > On 2012/10/24 08:18:15, xhwang wrote: > > I know this is from media code, but it seems to me this number is always an > > integer. How about use an int? When we do division, we can always cast to > double > > to preserve precision if needed. > > Done.
Looks good w/ one more comment. Did you test this? http://codereview.chromium.org/11260007/diff/2001/webkit/media/crypto/ppapi/f... File webkit/media/crypto/ppapi/ffmpeg_cdm_audio_decoder.h (right): http://codereview.chromium.org/11260007/diff/2001/webkit/media/crypto/ppapi/f... webkit/media/crypto/ppapi/ffmpeg_cdm_audio_decoder.h:34: // Returns |cdm::kDecodeError| when decoding fails. On 2012/10/24 21:16:44, Tom Finegan wrote: > On 2012/10/24 08:18:15, xhwang wrote: > > // Decodes |compressed_buffer|. > > // Returns |cdm::kSuccess| and stores output frame in |decoded_frames| when an > > output is available. > > // Returns |cdm::kNeedMoreData| when |compressed_frame| does not produce a*n* > > output. > > // Returns |cdm::kDecodeError| when decoding fails. > > I didn't use your wording, but your main concern seems to be that the first > thing the second second says is "Returns X". That's addressed, assuming you > agree with the new comment. :) ok http://codereview.chromium.org/11260007/diff/9001/webkit/media/crypto/ppapi/f... File webkit/media/crypto/ppapi/ffmpeg_cdm_audio_decoder.cc (right): http://codereview.chromium.org/11260007/diff/9001/webkit/media/crypto/ppapi/f... webkit/media/crypto/ppapi/ffmpeg_cdm_audio_decoder.cc:338: double decoded_us = (total_frames_decoded_ / samples_per_second_) * samples_per_second can be large, e.g. 44100. We could lose a lot of precision here. Do the multiplication first, or cast to double before doing division?
http://codereview.chromium.org/11260007/diff/9001/webkit/media/crypto/ppapi/f... File webkit/media/crypto/ppapi/ffmpeg_cdm_audio_decoder.cc (right): http://codereview.chromium.org/11260007/diff/9001/webkit/media/crypto/ppapi/f... webkit/media/crypto/ppapi/ffmpeg_cdm_audio_decoder.cc:338: double decoded_us = (total_frames_decoded_ / samples_per_second_) * On 2012/10/24 22:24:19, xhwang wrote: > samples_per_second can be large, e.g. 44100. We could lose a lot of precision > here. Do the multiplication first, or cast to double before doing division? Done. Are the consts OK?
On 2012/10/24 22:24:19, xhwang wrote: > Looks good w/ one more comment. Did you test this? Yes, and it worked (on the second try, because my code was lying and returning kSuccess for no output on the first try). I think I'm returning kNeedMoreData and kSuccess correctly now, and this version gets rid of IsEndOfOutputStream(). PTAL, thanks!
lgtm % nit http://codereview.chromium.org/11260007/diff/12003/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/ffmpeg_cdm_audio_decoder.cc (right): http://codereview.chromium.org/11260007/diff/12003/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_audio_decoder.cc:285: if (serialized_audio_frames_.size() > 0) { nit: !empty() http://codereview.chromium.org/11260007/diff/12003/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_audio_decoder.cc:300: return cdm::kNeedMoreData; I like this!
FYI, as far as I got before the new patch set. If these things are consistent with media/ we can ignore them. http://codereview.chromium.org/11260007/diff/13004/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/clear_key_cdm.cc (right): http://codereview.chromium.org/11260007/diff/13004/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/clear_key_cdm.cc:46: av_register_all(); Why is this necessary now? http://codereview.chromium.org/11260007/diff/13004/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/clear_key_cdm.cc:325: video_decoder_->Reset(); Fine for now, but I think we'll need to put the ifdefs in here when we address the mixing of fake decoders TODO http://codereview.chromium.org/11260007/diff/13004/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/ffmpeg_cdm_audio_decoder.h (right): http://codereview.chromium.org/11260007/diff/13004/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_audio_decoder.h:56: // Decoded audio format. Just "Audio format"? Decoded makes it sound like a per call thing. http://codereview.chromium.org/11260007/diff/13004/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_audio_decoder.h:66: // Number of output sample bytes to drop before generating Why would one want to do that?
http://codereview.chromium.org/11260007/diff/12003/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/clear_key_cdm.cc (right): http://codereview.chromium.org/11260007/diff/12003/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/clear_key_cdm.cc:46: av_register_all(); Do you just need a single place to call this? I don't think it actually needs to be done before the sandbox (or video decode would have been failing). The right thing to do is the cdm.h::InitializeModule() thing we discussed. At least put a TODO to move this out of here. I assume it is still necessary even after 91970 is fixed. Is that right? http://codereview.chromium.org/11260007/diff/12003/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.cc (left): http://codereview.chromium.org/11260007/diff/12003/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.cc:131: ReleaseFFmpegResources(); Do we not need this anymore? I don't see it being called.
Hopefully now CQ ready... just waiting for the fake audio decoder to land. http://codereview.chromium.org/11260007/diff/13004/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/clear_key_cdm.cc (right): http://codereview.chromium.org/11260007/diff/13004/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/clear_key_cdm.cc:46: av_register_all(); On 2012/10/25 01:27:18, ddorwin wrote: > Why is this necessary now? It needs to be called before any calls to avcodec_find_decoder(), but I don't know if it's safe to call multiple times. This seemed like the best place to put it since it's part of ffmpeg initialization. http://codereview.chromium.org/11260007/diff/13004/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/clear_key_cdm.cc:325: video_decoder_->Reset(); On 2012/10/25 01:27:18, ddorwin wrote: > Fine for now, but I think we'll need to put the ifdefs in here when we address > the mixing of fake decoders TODO sgtm http://codereview.chromium.org/11260007/diff/13004/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/ffmpeg_cdm_audio_decoder.h (right): http://codereview.chromium.org/11260007/diff/13004/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_audio_decoder.h:56: // Decoded audio format. On 2012/10/25 01:27:18, ddorwin wrote: > Just "Audio format"? > Decoded makes it sound like a per call thing. Done. http://codereview.chromium.org/11260007/diff/13004/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_audio_decoder.h:66: // Number of output sample bytes to drop before generating On 2012/10/25 01:27:18, ddorwin wrote: > Why would one want to do that? Because Vorbis and negative timestamps. Comment in .cc files explains a bit more, and includes a link to the spec. Added some more text to the comment. Note: from media. http://codereview.chromium.org/11260007/diff/12003/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/ffmpeg_cdm_audio_decoder.cc (right): http://codereview.chromium.org/11260007/diff/12003/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_audio_decoder.cc:285: if (serialized_audio_frames_.size() > 0) { On 2012/10/25 01:24:58, xhwang wrote: > nit: !empty() Done. http://codereview.chromium.org/11260007/diff/12003/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_audio_decoder.cc:300: return cdm::kNeedMoreData; On 2012/10/25 01:24:58, xhwang wrote: > I like this! It definitely seems to make this more readable. :)
http://codereview.chromium.org/11260007/diff/13004/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/clear_key_cdm.cc (right): http://codereview.chromium.org/11260007/diff/13004/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/clear_key_cdm.cc:46: av_register_all(); On 2012/10/25 01:42:19, Tom Finegan wrote: > On 2012/10/25 01:27:18, ddorwin wrote: > > Why is this necessary now? > > It needs to be called before any calls to avcodec_find_decoder(), but I don't > know if it's safe to call multiple times. This seemed like the best place to put > it since it's part of ffmpeg initialization. See my reply to the next revision. This is pre-sandbox (boo!). So, as I mentioned. Add a TODO to do it right.
Added TODO. Tree keeps closing... xhwang's patch is still waiting. Hopefully it'll open soon. http://codereview.chromium.org/11260007/diff/12003/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/clear_key_cdm.cc (right): http://codereview.chromium.org/11260007/diff/12003/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/clear_key_cdm.cc:46: av_register_all(); On 2012/10/25 01:34:17, ddorwin wrote: > Do you just need a single place to call this? I don't think it actually needs to > be done before the sandbox (or video decode would have been failing). > The right thing to do is the cdm.h::InitializeModule() thing we discussed. At > least put a TODO to move this out of here. I assume it is still necessary even > after 91970 is fixed. Is that right? Added TODO. Yes, the call will always be needed. http://codereview.chromium.org/11260007/diff/12003/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.cc (left): http://codereview.chromium.org/11260007/diff/12003/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/ffmpeg_cdm_video_decoder.cc:131: ReleaseFFmpegResources(); On 2012/10/25 01:34:17, ddorwin wrote: > Do we not need this anymore? I don't see it being called. It's called in ~FFmpegCdmVideoDecoder() and Deinitialize(), and is_initialized_ prevents it from being necessary to call it here.
lgtm Let's fix the module init next.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tomfinegan@chromium.org/11260007/6004
Change committed as 164162 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
