Add a media pipeline status error code for decryption error.

Add a media pipeline status error code for decryption error.

Also fix a bug in FFmpegVideoDecoderTest.DecodeEncryptedFrame_NoKey test that the frame isn't marked as decrypted.

BUG=121177
TEST=media_unittests


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=133397

[xhwang, 2012-04-20 19:08:28]

Hello scherkus,

This is the CL to add a separate decrypt error code. Please review.

xhwang

http://codereview.chromium.org/10165010/diff/1/media/filters/ffmpeg_video_dec...
File media/filters/ffmpeg_video_decoder_unittest.cc (right):

http://codereview.chromium.org/10165010/diff/1/media/filters/ffmpeg_video_dec...
media/filters/ffmpeg_video_decoder_unittest.cc:389: 
This is to fix a bug in the test. If we don't SetDecryptConfig() here, the frame
will be treated as a non-encrypted frame and trigger a decode error directly,
instead of a decryption error caused by no key.

[ddorwin, 2012-04-20 19:18:50]

http://codereview.chromium.org/10165010/diff/1/media/filters/ffmpeg_video_dec...
File media/filters/ffmpeg_video_decoder_unittest.cc (right):

http://codereview.chromium.org/10165010/diff/1/media/filters/ffmpeg_video_dec...
media/filters/ffmpeg_video_decoder_unittest.cc:389: 
On 2012/04/20 19:08:28, xhwang wrote:
> This is to fix a bug in the test. If we don't SetDecryptConfig() here, the
frame
> will be treated as a non-encrypted frame and trigger a decode error directly,
> instead of a decryption error caused by no key.

Should we copy the comment "// Simulate decoding a single encrypted frame." from
above? Needing to explain in the code review may be an indication that the code
needs a comment. (Of course, "this fixes the bug" is useful in describing the
change.)

http://codereview.chromium.org/10165010/diff/1/webkit/media/webmediaplayer_im...
File webkit/media/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/10165010/diff/1/webkit/media/webmediaplayer_im...
webkit/media/webmediaplayer_impl.cc:881: case media::PIPELINE_ERROR_DECRYPT:
WebKit now has a decrypt error. We should add a WebMediaPlayer function to send
it.

[xhwang, 2012-04-20 19:51:01]

http://codereview.chromium.org/10165010/diff/1/media/filters/ffmpeg_video_dec...
File media/filters/ffmpeg_video_decoder_unittest.cc (right):

http://codereview.chromium.org/10165010/diff/1/media/filters/ffmpeg_video_dec...
media/filters/ffmpeg_video_decoder_unittest.cc:389: 
On 2012/04/20 19:18:50, ddorwin wrote:
> On 2012/04/20 19:08:28, xhwang wrote:
> > This is to fix a bug in the test. If we don't SetDecryptConfig() here, the
> frame
> > will be treated as a non-encrypted frame and trigger a decode error
directly,
> > instead of a decryption error caused by no key.
> 
> Should we copy the comment "// Simulate decoding a single encrypted frame."
from
> above? Needing to explain in the code review may be an indication that the
code
> needs a comment. (Of course, "this fixes the bug" is useful in describing the
> change.)

Done.

http://codereview.chromium.org/10165010/diff/1/webkit/media/webmediaplayer_im...
File webkit/media/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/10165010/diff/1/webkit/media/webmediaplayer_im...
webkit/media/webmediaplayer_impl.cc:881: case media::PIPELINE_ERROR_DECRYPT:
On 2012/04/20 19:18:50, ddorwin wrote:
> WebKit now has a decrypt error. We should add a WebMediaPlayer function to
send
> it.

Do you mean adding something like NetworkStateDecodeError in WebMediaPlayer? In
that case I'll do it in another CL as it'll depend on a WebKit change. Correct
me if I am wrong (I am really new to WebKit).

[ddorwin, 2012-04-20 22:55:19]

http://codereview.chromium.org/10165010/diff/1/webkit/media/webmediaplayer_im...
File webkit/media/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/10165010/diff/1/webkit/media/webmediaplayer_im...
webkit/media/webmediaplayer_impl.cc:881: case media::PIPELINE_ERROR_DECRYPT:
On 2012/04/20 19:51:01, xhwang wrote:
> On 2012/04/20 19:18:50, ddorwin wrote:
> > WebKit now has a decrypt error. We should add a WebMediaPlayer function to
> send
> > it.
> 
> Do you mean adding something like NetworkStateDecodeError in WebMediaPlayer?
In
> that case I'll do it in another CL as it'll depend on a WebKit change. Correct
> me if I am wrong (I am really new to WebKit).

Right. I looked at the WebKit code more and found that we just need to add a new
NetworkStateDecryptError. This will require WebKit changes and minor Chromium
changes. I filed crbug.com/124486.

I suggest breaking this case out, copying the code below, and putting a TODO
that refers to the new bug.

[xhwang, 2012-04-21 00:53:01]

http://codereview.chromium.org/10165010/diff/1/webkit/media/webmediaplayer_im...
File webkit/media/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/10165010/diff/1/webkit/media/webmediaplayer_im...
webkit/media/webmediaplayer_impl.cc:881: case media::PIPELINE_ERROR_DECRYPT:
On 2012/04/20 22:55:19, ddorwin wrote:
> On 2012/04/20 19:51:01, xhwang wrote:
> > On 2012/04/20 19:18:50, ddorwin wrote:
> > > WebKit now has a decrypt error. We should add a WebMediaPlayer function to
> > send
> > > it.
> > 
> > Do you mean adding something like NetworkStateDecodeError in WebMediaPlayer?
> In
> > that case I'll do it in another CL as it'll depend on a WebKit change.
Correct
> > me if I am wrong (I am really new to WebKit).
> 
> Right. I looked at the WebKit code more and found that we just need to add a
new
> NetworkStateDecryptError. This will require WebKit changes and minor Chromium
> changes. I filed crbug.com/124486.
> 
> I suggest breaking this case out, copying the code below, and putting a TODO
> that refers to the new bug.

Done.

[scherkus (not reviewing), 2012-04-21 03:04:22]

LGTM

nice!!!

http://codereview.chromium.org/10165010/diff/3007/media/filters/ffmpeg_video_...
File media/filters/ffmpeg_video_decoder_unittest.cc (right):

http://codereview.chromium.org/10165010/diff/3007/media/filters/ffmpeg_video_...
media/filters/ffmpeg_video_decoder_unittest.cc:417: // the two. See:
crbug.com/124434.
nit: want to add http:// and remove the trailing . ?

some editors include the trailing dot as part of the URL :\

http://codereview.chromium.org/10165010/diff/3007/webkit/media/webmediaplayer...
File webkit/media/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/10165010/diff/3007/webkit/media/webmediaplayer...
webkit/media/webmediaplayer_impl.cc:893: // Webkit. See crbug.com/124486.
ditto here

[commit-bot: I haz the power, 2012-04-21 18:31:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xhwang@chromium.org/10165010/7003

[commit-bot: I haz the power, 2012-04-21 20:46:16]

Try job failure for 10165010-7003 (retry) on win_rel for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-04-23 01:03:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xhwang@chromium.org/10165010/7003

[commit-bot: I haz the power, 2012-04-23 02:20:58]

Change committed as 133397