third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-precise-duration.html - Issue 2581533002: MSE: Fix logic bugs with high precision duration

Side by Side Diff: third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-precise-duration.html

Issue 2581533002: MSE: Fix logic bugs with high precision duration (Closed)

Patch Set: Created 4 years ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

« media/filters/chunk_demuxer.cc ('K') | « media/filters/chunk_demuxer.cc ('k') | third_party/WebKit/Source/core/html/HTMLMediaElement.cpp » ('j') | third_party/WebKit/Source/core/html/HTMLMediaElement.cpp » ('J')
Toggle Intra-line Diffs ('i') | Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')

OLD	NEW
(Empty)
	1 <!DOCTYPE html>

	2 <html>

	3 <head>

	4 <script src="/w3c/resources/testharness.js"></script>

	5 <script src="/w3c/resources/testharnessreport.js"></script>

	6 <script src="mediasource-util.js"></script>

	7 <link rel='stylesheet' href='/w3c/resources/testharness.css'>
	wolenetz 2016/12/16 00:56:52 this isn't necessary any longer this isn't necessary any longer chcunningham 2017/01/03 17:48:05 Done. Show quoted text On 2016/12/16 00:56:52, wolenetz wrote: > this isn't necessary any longer Done.
	8 </head>

	9 <body>

	10 <div id="log"></div>

	11 <script>

	12 mediasource_testafterdataloaded(function(test, mediaElement, mediaSour ce, segmentInfo, sourceBuffer, mediaData)

	13 {

	14 // Set duration .5 nanoseconds higher than known duration. Interna lly, most user

	15 // agents do not represent media time with sub-nanosecond precisio n, so this attempts

	16 // to catch edge cases arising from data type conversions.

	17 let increased_duration = segmentInfo.duration + /* .5 nanoseconds */ .0000000005;
	mlamouri (slow - plz ping) 2016/12/15 10:20:51 Unless all mediasource-* tests are using `let`, I Unless all mediasource-* tests are using `let`, I think using `var` should be better in order to make the test re-usable by other browsers. chcunningham 2017/01/03 17:48:05 We have a lot of let usage already. https://cs.chr Show quoted text On 2016/12/15 10:20:51, mlamouri wrote: > Unless all mediasource-* tests are using `let`, I think using `var` should be > better in order to make the test re-usable by other browsers. We have a lot of let usage already. https://cs.chromium.org/search/?q=f:LayoutTests+%22let+%22&sq=package:chromiu...
	18 mediaSource.duration = increased_duration;

	19

	20 // Expect duration to exactly match what was set.

	21 assert_equals(mediaSource.duration, increased_duration);

	22

	23 // Append media data.

	24 test.expectEvent(sourceBuffer, "updateend");

	25 sourceBuffer.appendBuffer(mediaData);

	26 test.waitForExpectedEvents(function()
	mlamouri (slow - plz ping) 2016/12/15 10:20:51 Could this simply be: ``` sourceBuffer.addEventLis Could this simply be: ``` sourceBuffer.addEventListener('updateend', test.step_func(function() { }); ``` (ie. no need for `expectEvent` and `waitForExpectedEvents` with just one event) chcunningham 2017/01/03 17:48:05 I started down this path but it makes the test les Show quoted text On 2016/12/15 10:20:51, mlamouri wrote: > Could this simply be: > ``` > sourceBuffer.addEventListener('updateend', test.step_func(function() { > }); > ``` > > (ie. no need for `expectEvent` and `waitForExpectedEvents` with just one event) I started down this path but it makes the test less readable because the functions that handle the events must then be defined above the code that fires the events. I thought about working around that by having the event handlers just resolve a promise which I could hook into below, but thats just re-inventing the expectEvent/waitForExpectedEvents infra.
	27 {

	28 // Appending all data should not change the duration because s et duration is

	29 // already larger than segmentInfo metadata (and we trust segm entInfo has an

	30 // accurate duration).
	wolenetz 2016/12/16 00:56:52 Add comment reference to existing (or new) test th Add comment reference to existing (or new) test that demonstrates this trust please :) I'm pretty sure we have such a test downstream already, but documenting here would help. chcunningham 2017/01/03 17:48:05 Done. Show quoted text On 2016/12/16 00:56:52, wolenetz wrote: > Add comment reference to existing (or new) test that demonstrates this trust > please :) > I'm pretty sure we have such a test downstream already, but documenting here > would help. Done.
	31 assert_equals(mediaSource.duration, increased_duration);

	32

	33 // Marking end of stream should cause duration to be reduced t o match highest

	34 // buffered end time.

	35 mediaSource.endOfStream();

	36

	37 assert_equals(mediaElement.buffered.length, 1);

	38 assert_equals(mediaSource.duration, mediaElement.buffered.end( 0));

	39

	40 // Play the last half second of media to verify 'ended' event is observed. The

	41 // ended event should be triggered when currentTime >= duratio n, so ensure that

	42 // earlier use of high-precision does not prevent this.

	43 assert_true(mediaSource.duration > .5);
	mlamouri (slow - plz ping) 2016/12/15 10:20:52 assert_greater_than(mediaSource.duration, .5); assert_greater_than(mediaSource.duration, .5); chcunningham 2017/01/03 17:48:05 Done. Show quoted text On 2016/12/15 10:20:52, mlamouri wrote: > assert_greater_than(mediaSource.duration, .5); Done.
	44 mediaElement.currentTime = mediaSource.duration - .5;

	45

	46 test.expectEvent(mediaElement, "ended");

	47 mediaElement.play();

	48

	49 test.waitForExpectedEvents(function()
	mlamouri (slow - plz ping) 2016/12/15 10:20:51 same as above but I think you can even right a one same as above but I think you can even right a one liner: ``` mediaElement.onended = test.step_func_done(); ``` chcunningham 2017/01/03 17:48:05 simplified, but continued using the waitFor. See c Show quoted text On 2016/12/15 10:20:51, mlamouri wrote: > same as above but I think you can even right a one liner: > ``` > mediaElement.onended = test.step_func_done(); > ``` simplified, but continued using the waitFor. See comment above.
	50 {

	51 test.done();

	52 })

	53 });

	54 }, "Test high precision duration is returned, then truncated upon endO fStream()");

	55 </script>

	56 </body>

	57 </html>

OLD	NEW