MSE: Add layout test for 'decode' error via an algorithm

MSE: Add layout test for 'decode' error via an algorithm

BUG=

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=189078

[jiajia.qin, 2014-11-19 06:46:43]

jiajia.qin@intel.com changed reviewers:
+ philipj@opera.com

[jiajia.qin, 2014-11-19 06:46:43]

PTAL. Related with https://codereview.chromium.org/710693003/.

[philipj_slow, 2014-11-19 10:08:35]

I've verified that this fails without the Chromium change and passes with it. I
think a 3-sided patch will not be needed, just land the Chromium side first and
then land this soon after.

https://codereview.chromium.org/742653002/diff/1/LayoutTests/http/tests/media...
File LayoutTests/http/tests/media/media-source/mediasource-errors.html (right):

https://codereview.chromium.org/742653002/diff/1/LayoutTests/http/tests/media...
LayoutTests/http/tests/media/media-source/mediasource-errors.html:153: var index
= (mediaSegment.length + 1) / 5;
This and the follow line looks a bit odd, like some finely tuned constants that
need to be documented. I think this is trying to create a buffer that includes
an init segment followed by some corrupt data, right? It looks like mediaData is
already the concatenation of init segment and media data, so why not just
overwrite some of the media data to make it corrupt? I haven't looked at many
MSE test cases before, so I could just be ignorant about how things work.

[jiajia.qin, 2014-11-19 11:57:57]

https://codereview.chromium.org/742653002/diff/1/LayoutTests/http/tests/media...
File LayoutTests/http/tests/media/media-source/mediasource-errors.html (right):

https://codereview.chromium.org/742653002/diff/1/LayoutTests/http/tests/media...
LayoutTests/http/tests/media/media-source/mediasource-errors.html:153: var index
= (mediaSegment.length + 1) / 5;
On 2014/11/19 10:08:35, philipj wrote:
> This and the follow line looks a bit odd, like some finely tuned constants
that
> need to be documented. I think this is trying to create a buffer that includes
> an init segment followed by some corrupt data, right?
Yes. I am trying to create a buffer that includes an int segment, partial media
segment(the first 1/5 data is right, but remaining is corrupt data)
> It looks like mediaData is
> already the concatenation of init segment and media data, so why not just
> overwrite some of the media data to make it corrupt?
I can overwrite some of the media data. But I don't know what kind of data I
should use. Can I just overwrite it using arbitrary number ?  
> I haven't looked at many
> MSE test cases before, so I could just be ignorant about how things work.
The test cases before are explicitly calling mediaSource.endofstream() to
trigger the end of stream algorithm. This test case is triggered by an algorithm
which needs to signal a decode error.
( https://w3c.github.io/media-source/#end-of-stream-algorithm )

[philipj_slow, 2014-11-19 12:22:37]

I'd try simply writing zeros in some part of the media data. Or inverting some
bits. Any corruption which is enough to trigger a decode error will do.

With the existing code, what does the resulting buffer actually look like? Is it
an init segment followed by 1/5 of a media segment, or is there something
additional after that?

[jiajia.qin, 2014-11-20 08:15:06]

On 2014/11/19 12:22:37, philipj wrote:
> I'd try simply writing zeros in some part of the media data. Or inverting some
> bits. Any corruption which is enough to trigger a decode error will do.
> 
> With the existing code, what does the resulting buffer actually look like? Is
it
> an init segment followed by 1/5 of a media segment, or is there something
> additional after that?
mediaData is a Uint8Array type. mediaData.set(array, offset) means
copy items from array into mediaData starting at mediaData[offset].
The Patch Set 1 looks confused since it is a little complicated.

[philipj_slow, 2014-11-20 09:02:02]

https://codereview.chromium.org/742653002/diff/20001/LayoutTests/http/tests/m...
File LayoutTests/http/tests/media/media-source/mediasource-errors.html (right):

https://codereview.chromium.org/742653002/diff/20001/LayoutTests/http/tests/m...
LayoutTests/http/tests/media/media-source/mediasource-errors.html:152: var index
= segmentInfo.init.size + (mediaSegment.length + 1) / 2;
I don't understand the +1 here. The lowest index of the media segment ought to
be "segmentInfo.init.size" and the highest index "segmentInfo.init.size +
mediaSegment.length - 1", so to scale between those with a factor [0,1] (in this
case 0.5) the formula would be "segmentInfo.init.size + (mediaSegment.length -
1) / 2".

If there's some other intention to this formula, please document it :)

https://codereview.chromium.org/742653002/diff/20001/LayoutTests/http/tests/m...
LayoutTests/http/tests/media/media-source/mediasource-errors.html:154: // Here
use mediaSegment to replace the original mediaData[index, index +
mediaSegment.length]
If I'm reading this correctly, we'll the init segment, half of the media segment
and then the media segment again? That sounds corrupt enough to me :)

[jiajia.qin, 2014-11-20 10:12:24]

https://codereview.chromium.org/742653002/diff/20001/LayoutTests/http/tests/m...
File LayoutTests/http/tests/media/media-source/mediasource-errors.html (right):

https://codereview.chromium.org/742653002/diff/20001/LayoutTests/http/tests/m...
LayoutTests/http/tests/media/media-source/mediasource-errors.html:152: var index
= segmentInfo.init.size + (mediaSegment.length + 1) / 2;
On 2014/11/20 09:02:02, philipj wrote:
> I don't understand the +1 here. The lowest index of the media segment ought to
> be "segmentInfo.init.size" and the highest index "segmentInfo.init.size +
> mediaSegment.length - 1", so to scale between those with a factor [0,1] (in
this
> case 0.5) the formula would be "segmentInfo.init.size + (mediaSegment.length -
> 1) / 2".
> 
I just follow 'mediasource-append-buffer.html'. In that file's test case, use
'var harfIndex = (mediaSegment.length + 1) / 2'.
But I think no matter +1 or -1 won't affect this test. Here, I just random
select a point to corrupt the data.
> If there's some other intention to this formula, please document it :)

https://codereview.chromium.org/742653002/diff/20001/LayoutTests/http/tests/m...
LayoutTests/http/tests/media/media-source/mediasource-errors.html:154: // Here
use mediaSegment to replace the original mediaData[index, index +
mediaSegment.length]
On 2014/11/20 09:02:02, philipj wrote:
> If I'm reading this correctly, we'll the init segment, half of the media
segment
> and then the media segment again? That sounds corrupt enough to me :)
Yes, it is :)

[philipj_slow, 2014-11-20 10:45:45]

https://codereview.chromium.org/742653002/diff/20001/LayoutTests/http/tests/m...
File LayoutTests/http/tests/media/media-source/mediasource-errors.html (right):

https://codereview.chromium.org/742653002/diff/20001/LayoutTests/http/tests/m...
LayoutTests/http/tests/media/media-source/mediasource-errors.html:152: var index
= segmentInfo.init.size + (mediaSegment.length + 1) / 2;
On 2014/11/20 10:12:23, jiajia.qin wrote:
> On 2014/11/20 09:02:02, philipj wrote:
> > I don't understand the +1 here. The lowest index of the media segment ought
to
> > be "segmentInfo.init.size" and the highest index "segmentInfo.init.size +
> > mediaSegment.length - 1", so to scale between those with a factor [0,1] (in
> this
> > case 0.5) the formula would be "segmentInfo.init.size + (mediaSegment.length
-
> > 1) / 2".
> > 
> I just follow 'mediasource-append-buffer.html'. In that file's test case, use
> 'var harfIndex = (mediaSegment.length + 1) / 2'.
> But I think no matter +1 or -1 won't affect this test. Here, I just random
> select a point to corrupt the data.
> > If there's some other intention to this formula, please document it :)
> 

In that case, please use - 1 instead in both tests so that future readers don't
have to wonder if there's something special going on.

[jiajia.qin, 2014-11-20 11:00:26]

On 2014/11/20 10:45:45, philipj wrote:
>
https://codereview.chromium.org/742653002/diff/20001/LayoutTests/http/tests/m...
> File LayoutTests/http/tests/media/media-source/mediasource-errors.html
(right):
> 
>
https://codereview.chromium.org/742653002/diff/20001/LayoutTests/http/tests/m...
> LayoutTests/http/tests/media/media-source/mediasource-errors.html:152: var
index
> = segmentInfo.init.size + (mediaSegment.length + 1) / 2;
> On 2014/11/20 10:12:23, jiajia.qin wrote:
> > On 2014/11/20 09:02:02, philipj wrote:
> > > I don't understand the +1 here. The lowest index of the media segment
ought
> to
> > > be "segmentInfo.init.size" and the highest index "segmentInfo.init.size +
> > > mediaSegment.length - 1", so to scale between those with a factor [0,1]
(in
> > this
> > > case 0.5) the formula would be "segmentInfo.init.size +
(mediaSegment.length
> -
> > > 1) / 2".
> > > 
> > I just follow 'mediasource-append-buffer.html'. In that file's test case,
use
> > 'var harfIndex = (mediaSegment.length + 1) / 2'.
> > But I think no matter +1 or -1 won't affect this test. Here, I just random
> > select a point to corrupt the data.
> > > If there's some other intention to this formula, please document it :)
> > 
> 
> In that case, please use - 1 instead in both tests so that future readers
don't
> have to wonder if there's something special going on.

You mean I can put the change for other tests into this CL together?

[philipj_slow, 2014-11-20 12:04:33]

Yeah, assuming it doesn't actually change the results of those tests, I'm OK
with that. Or if you want to only change this test, I can change the rest in a
separate CL.

[jiajia.qin, 2014-11-20 12:19:35]

On 2014/11/20 12:04:33, philipj wrote:
> Yeah, assuming it doesn't actually change the results of those tests, I'm OK
> with that. Or if you want to only change this test, I can change the rest in a
> separate CL.

Done it since it's a simple thing. Thanks.

[philipj_slow, 2014-11-20 12:45:48]

OK, this test LGTM.

[wolenetz, 2015-01-26 21:11:51]

wolenetz@chromium.org changed reviewers:
+ wolenetz@chromium.org

[wolenetz, 2015-01-26 21:11:51]

drive by lgtm w.r.t. https://codereview.chromium.org/710693003/

Thanks for fixing this!

[jiajia.qin, 2015-01-28 01:23:17]

The CQ bit was checked by jiajia.qin@intel.com

[commit-bot: I haz the power, 2015-01-28 01:24:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/742653002/40001

[commit-bot: I haz the power, 2015-01-28 02:38:08]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=189078