Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
544173012:
Add LayoutTest for media with positive start times. (Closed)
|
Created:
6 years, 3 months ago by DaleCurtis Modified:
6 years, 3 months ago CC:
blink-reviews, feature-media-reviews_chromium.org, eric.carlson_apple.com Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Project:
blink Visibility:
Public. |
DescriptionAdd LayoutTest for media with positive start times.
BUG=413292
TEST=this!
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=182286
Patch Set 1 #
Total comments: 3
Patch Set 2 : Style. #
Total comments: 2
Patch Set 3 : testharness.js #
Total comments: 5
Patch Set 4 : Moar tests! #Patch Set 5 : Better video! #
Messages
Total messages: 23 (4 generated)
dalecurtis@chromium.org changed reviewers: + acolwell@chromium.org
As previously discussed.
Won't pass until after https://codereview.chromium.org/575643002/ lands.
lgtm % nits https://codereview.chromium.org/544173012/diff/1/LayoutTests/media/video-posi... File LayoutTests/media/video-positive-start-time.html (right): https://codereview.chromium.org/544173012/diff/1/LayoutTests/media/video-posi... LayoutTests/media/video-positive-start-time.html:6: <script src="video-test.js"></script> nit: Please put all the script in a <head> and indent the HTML. https://codereview.chromium.org/544173012/diff/1/LayoutTests/media/video-posi... LayoutTests/media/video-positive-start-time.html:9: waitForEventOnce('loadeddata', function() { nit: { should be on the next line. https://codereview.chromium.org/544173012/diff/1/LayoutTests/media/video-posi... LayoutTests/media/video-positive-start-time.html:16: function start() { ditto
I've indented, but have not added the <head> tag or made the other changes. The new layout test style guideline says Chromium style bracing is okay and to leave out <head>,<html>,<body> if not required. http://www.chromium.org/blink/coding-style/layout-test-style-guidelines
philipj@opera.com changed reviewers: + philipj@opera.com
The new test-positive-start-time.webm is 36K which will forever be part of the repo size. Admittedly it's already smaller than the other test files, but did you try minimizing it by having only white frames? If this file isn't going to be used in reftests, it doesn't matter what the content is. https://codereview.chromium.org/544173012/diff/20001/LayoutTests/media/video-... File LayoutTests/media/video-positive-start-time.html (right): https://codereview.chromium.org/544173012/diff/20001/LayoutTests/media/video-... LayoutTests/media/video-positive-start-time.html:8: <script src="video-test.js"></script> You can't really put anything after </body>, the HTML parser will simply move it to inside <body>. I would like new tests to use testharness.js wherever possible, and this is how this test would translate: <!DOCTYPE html> <title>video with a postive start time</title> <script src="../resources/testharness.js"></script> <script src="../resources/testharnessreport.js"></script> <div id="log"></div> <video></video> <script> async_test(function(t) { var video = document.querySelector('video'); video.src = 'resources/test-positive-start-time.webm'; video.onloadeddata = t.step_func(function() { assert_equals(video.currentTime, 4.253, 'currentTime'); // FIXME: Once Chrome correctly exposes seekable ranges for media with // positive start times, verify video.seekable.start(0) here. t.done(); }); }); </script> No expectation file is then needed. Please also add a test using 'resources/test-positive-start-time.webm#t=5' and assert that the correct offset wins, which per spec is 5.
I looked at recreating the video, but I was unable to generate a solid color video with a large start time. For whatever reason, ffmpeg won't snap to the start time desired. Here's the command lines I'm using if you have any tips: ffmpeg -f lavfi -i color=color=white -i test.mp4 -map 0:0:1 -map 1:0:1 -vcodec vp8 -keyint_min 1 -acodec vorbis -t 6 -strict -2 white.webm ffmpeg -copyts -ss 6 -i white.webm -ss 4 -acodec copy -vcodec copy test-positive-start-time.webm Gives: Duration: 00:00:02.04, start: 1.120000 Versus the current version: Duration: 00:00:06.05, start: 4.253000, Which gives some more leeway for seeking about. In any case, a pure white video of the same duration with the audio track is about ~7.8k in size, which while saves us a couple tens of kb, is not the end of the world. If you feel strongly I can swap them out and change the numbers in the test. https://codereview.chromium.org/544173012/diff/20001/LayoutTests/media/video-... File LayoutTests/media/video-positive-start-time.html (right): https://codereview.chromium.org/544173012/diff/20001/LayoutTests/media/video-... LayoutTests/media/video-positive-start-time.html:8: <script src="video-test.js"></script> On 2014/09/17 11:25:36, philipj wrote: > You can't really put anything after </body>, the HTML parser will simply move it > to inside <body>. > > I would like new tests to use testharness.js wherever possible, and this is how > this test would translate: > > <!DOCTYPE html> > <title>video with a postive start time</title> > <script src="../resources/testharness.js"></script> > <script src="../resources/testharnessreport.js"></script> > <div id="log"></div> > <video></video> > <script> > async_test(function(t) { > var video = document.querySelector('video'); > video.src = 'resources/test-positive-start-time.webm'; > video.onloadeddata = t.step_func(function() { > assert_equals(video.currentTime, 4.253, 'currentTime'); > // FIXME: Once Chrome correctly exposes seekable ranges for media with > // positive start times, verify video.seekable.start(0) here. > t.done(); > }); > }); > </script> > > No expectation file is then needed. > > Please also add a test using 'resources/test-positive-start-time.webm#t=5' and > assert that the correct offset wins, which per spec is 5. Done. Thanks for the drive-by!
https://codereview.chromium.org/544173012/diff/40001/LayoutTests/media/video-... File LayoutTests/media/video-positive-start-time.html (right): https://codereview.chromium.org/544173012/diff/40001/LayoutTests/media/video-... LayoutTests/media/video-positive-start-time.html:16: console.log(video.currentTime); Why console.log here? Will it should up somewhere? https://codereview.chromium.org/544173012/diff/40001/LayoutTests/media/video-... LayoutTests/media/video-positive-start-time.html:18: video.currentTime = seekTime; Verifying that seeking works as expected is good, but I would perhaps doing it in a separate test or tests. If you haven't already, have a look at the steps for "Once enough of the media data has been fetched to determine the duration of the media resource, its dimensions, and other metadata" in the spec: https://html.spec.whatwg.org/#media-data-processing-steps-list Here the "earliest possible position", "default playback start position " and "initial playback position" interact, so I think testing should focus on this algorithm. In other words: - One test that sets currentTime = 5 right before or after src - One test that sets src to 'resources/test-positive-start-time.webm#t=2' Both should have currentTime 5 in the loadedmetadata event, which should be followed by a seeked event.
https://codereview.chromium.org/544173012/diff/40001/LayoutTests/media/video-... File LayoutTests/media/video-positive-start-time.html (right): https://codereview.chromium.org/544173012/diff/40001/LayoutTests/media/video-... LayoutTests/media/video-positive-start-time.html:16: console.log(video.currentTime); On 2014/09/18 20:07:43, philipj wrote: > Why console.log here? Will it should up somewhere? Whoops, that's an accidental include on my part. Sorry. https://codereview.chromium.org/544173012/diff/40001/LayoutTests/media/video-... LayoutTests/media/video-positive-start-time.html:18: video.currentTime = seekTime; On 2014/09/18 20:07:43, philipj wrote: > Verifying that seeking works as expected is good, but I would perhaps doing it > in a separate test or tests. If you haven't already, have a look at the steps > for "Once enough of the media data has been fetched to determine the duration of > the media resource, its dimensions, and other metadata" in the spec: > https://html.spec.whatwg.org/#media-data-processing-steps-list > > Here the "earliest possible position", "default playback start position " and > "initial playback position" interact, so I think testing should focus on this > algorithm. > > In other words: > > - One test that sets currentTime = 5 right before or after src > - One test that sets src to 'resources/test-positive-start-time.webm#t=2' > > Both should have currentTime 5 in the loadedmetadata event, which should be > followed by a seeked event. I had trouble running multiple tests in the same file since the tests run concurrently and use the same video tag. Are you suggesting creating a separate test file or chaining them based upon completion somehow? Hmm, why would the currentTime=2 end up with a currentTime of 5? It should be the earliest available position which is 4.253 no?
https://codereview.chromium.org/544173012/diff/40001/LayoutTests/media/video-... File LayoutTests/media/video-positive-start-time.html (right): https://codereview.chromium.org/544173012/diff/40001/LayoutTests/media/video-... LayoutTests/media/video-positive-start-time.html:18: video.currentTime = seekTime; On 2014/09/18 20:17:12, DaleCurtis wrote: > On 2014/09/18 20:07:43, philipj wrote: > > Verifying that seeking works as expected is good, but I would perhaps doing it > > in a separate test or tests. If you haven't already, have a look at the steps > > for "Once enough of the media data has been fetched to determine the duration > of > > the media resource, its dimensions, and other metadata" in the spec: > > https://html.spec.whatwg.org/#media-data-processing-steps-list > > > > Here the "earliest possible position", "default playback start position " and > > "initial playback position" interact, so I think testing should focus on this > > algorithm. > > > > In other words: > > > > - One test that sets currentTime = 5 right before or after src > > - One test that sets src to 'resources/test-positive-start-time.webm#t=2' > > > > Both should have currentTime 5 in the loadedmetadata event, which should be > > followed by a seeked event. > > I had trouble running multiple tests in the same file since the tests run > concurrently and use the same video tag. Are you suggesting creating a separate > test file or chaining them based upon completion somehow? My thought was simply having multiple files, but if you ever want to have multiple tests in the same file, you can create a video element per test. I wouldn't really recommend it though, when such tests are flaky it makes it harder to figure out the cause. > Hmm, why would the currentTime=2 end up with a currentTime of 5? It should be > the earliest available position which is 4.253 no? Sorry, I meant currentTime=5 :)
Patchset #4 (id:60001) has been deleted
Okay, split out into three tests, one w/o a seek, one w/ before, and one w/ after.
About the file, my old swiss army knife of choice is GStreamer's command line tools. I tried this: gst-launch videotestsrc pattern=white timestamp-offset=3000000000 num-buffers=50 ! video/x-raw-yuv, framerate=10/1 ! vp8enc quality=0 ! queue2 ! webmmux ! queue2 ! filesink location=test-positive-start-time.webm That results in a 5 second long file that starts at 3 seconds and is 2150 bytes. It could be made smaller by lowering the framerate more, but meh. Here it is: http://foolip.org/2014/09/18/test-positive-start-time.webm Can you check if that has the required properties for these tests?
I bow before your superior swiss army knife! I'll have to check out gstreamer, I've always just stuck with ffmpeg.
The tests look good now. With "One test that sets currentTime = 5 right before or after src" I meant video.currentTime=5 and not #t=5, but I think this is OK, we have other tests to check which of "default playback start position" and a media fragment will win.
On 2014/09/18 20:57:53, DaleCurtis wrote: > I bow before your superior swiss army knife! I'll have to check out gstreamer, > I've always just stuck with ffmpeg. Cool, I wasn't sure it would actually work. What tool did you use to get the "Duration: 00:00:02.04, start: 1.120000" info from the other file and what does it say about mine?
Anyway, LGTM to land if the tests now pass!
On 2014/09/18 21:00:21, philipj wrote: > On 2014/09/18 20:57:53, DaleCurtis wrote: > > I bow before your superior swiss army knife! I'll have to check out gstreamer, > > I've always just stuck with ffmpeg. > > Cool, I wasn't sure it would actually work. What tool did you use to get the > "Duration: 00:00:02.04, start: 1.120000" info from the other file and what does > it say about mine? That was just "ffmpeg -i <file>" and yours reports: Duration: 00:00:05.00, start: 3.000000
On 2014/09/18 21:01:19, DaleCurtis wrote: > On 2014/09/18 21:00:21, philipj wrote: > > On 2014/09/18 20:57:53, DaleCurtis wrote: > > > I bow before your superior swiss army knife! I'll have to check out > gstreamer, > > > I've always just stuck with ffmpeg. > > > > Cool, I wasn't sure it would actually work. What tool did you use to get the > > "Duration: 00:00:02.04, start: 1.120000" info from the other file and what > does > > it say about mine? > > That was just "ffmpeg -i <file>" and yours reports: > > Duration: 00:00:05.00, start: 3.000000 Thanks, I never did learn how to use ffmpeg :)
The CQ bit was checked by dalecurtis@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/544173012/100001
Message was sent while issue was closed.
Committed patchset #5 (id:100001) as 182286 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
