Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10647007:
Add http based media browser tests. (Closed)
|
Created:
8 years, 6 months ago by DaleCurtis Modified:
8 years, 4 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, jochen+watch-content_chromium.org, joi+watch-content_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, pam+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd http based media browser tests.
In response to the recent media failure (http://crbug.com/133808) we need
tests which actually run over http and request enough data to trigger the
more complex behaviors of our http loader.
In this CL I've modified all the existing tests to wait for playthrough
over both file:// and http:// protocols. Additionally I've checked in a
large (~4.2MB) test file (with nsylvain's approval) for triggering the
buffering issue in the bug mentioned above.
BUG=134034
TEST=These tests. Eventually try bots.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=150208
Patch Set 1 #
Total comments: 4
Patch Set 2 : Remove wave generation. Use static file. #
Total comments: 8
Patch Set 3 : Rebase. Try disabling audio. #
Total comments: 11
Patch Set 4 : Use seek to reduce test time. #
Total comments: 2
Patch Set 5 : Replace setTimeout w/ timeupdate events. #
Total comments: 4
Patch Set 6 : Docs! #
Total comments: 4
Patch Set 7 : Overrides. #Patch Set 8 : Rebase. #
Messages
Total messages: 40 (0 generated)
scherkus: full review. akalin: testserver.py changes. sky: content/ rubberstamp or additional review.
willchan is a better OWNERS reviewer than me -- I only do the sync-related bits for test_server.
From a layering perspective, adding support for waveform generation in the net/ codebase feels a bit off. That being said, testserver is quite a bit different than the actual net library itself.
On 2012/06/22 14:03:51, cbentzel wrote: > From a layering perspective, adding support for waveform generation in the net/ > codebase feels a bit off. > > That being said, testserver is quite a bit different than the actual net library > itself. Why can't you just add wave data to chrome/test/data/media? Does it need to be randomly generated each time?
On 2012/06/22 14:05:37, cbentzel wrote: > On 2012/06/22 14:03:51, cbentzel wrote: > > From a layering perspective, adding support for waveform generation in the > net/ > > codebase feels a bit off. > > > > That being said, testserver is quite a bit different than the actual net > library > > itself. > > Why can't you just add wave data to chrome/test/data/media? Does it need to be > randomly generated each time? It's just a size thing. If everyone is okay with checking in a 4MB+ WAVE (possibly more in the future) there that's fine with me. Alternatively, we've got a private avperf repo with our truly large test files and some "smaller" (~15-30mb files) ones in pyauto_private already. If we could add either repo to the standard build / try check out that's another solution. Since this test needs to run on the normal build / try bots, most (all?) of which don't check out either of those repos I figured auto-generation was the best way to go.
nsylvain: Is it reasonable to add a 4 MB binary file to chrome\test\data? Will this impact Update steps? Anything else to be concerned about?
On 2012/06/23 01:42:51, cbentzel wrote: > nsylvain: Is it reasonable to add a 4 MB binary file to chrome\test\data? Will > this impact Update steps? Anything else to be concerned about? Just one file? Used on all platforms? Does not change too frequently? (no more than once a month?) If so, that should be fine.
On 2012/06/25 21:01:15, nsylvain wrote: > On 2012/06/23 01:42:51, cbentzel wrote: > > nsylvain: Is it reasonable to add a 4 MB binary file to chrome\test\data? Will > > this impact Update steps? Anything else to be concerned about? > > Just one file? Used on all platforms? Does not change too frequently? (no more > than once a month?) If so, that should be fine. It is one file now, which would not change in the foreseeable future, but we may want more files later. It's just circumstance that the bug these tests are in response to triggered more frequently with 4MB of data versus larger test data. It's very possible we'll want a test with 20MB of data in the near future. Having flexibility for ~20 lines of code (minus comments) vs checking in multi-megabyte static files seems a better route to go.
I'm pretty busy with conference stuff. Matt/Chris, can one of you look at this? If not, punt back to me and I'll try to get to it in due time.
Also busy at conference, but looked at it and responded. I think for now I'd just do a static file. If this ends up being used multiple times in the future, let's do the handler approach then. On Tue, Jun 26, 2012 at 1:53 AM, <willchan@chromium.org> wrote: > I'm pretty busy with conference stuff. Matt/Chris, can one of you look at > this? > If not, punt back to me and I'll try to get to it in due time. > > https://chromiumcodereview.**appspot.com/10647007/<https://chromiumcodereview... >
-bunch of people. Landed 4.2mb test data with http://src.chromium.org/viewvc/chrome?view=rev&revision=146253 scherkus: Pleas review. sky: Please OWNERS stamp.
http://codereview.chromium.org/10647007/diff/13001/chrome/test/data/media/pla... File chrome/test/data/media/player.html (right): http://codereview.chromium.org/10647007/diff/13001/chrome/test/data/media/pla... chrome/test/data/media/player.html:15: var url_parts = [url.substring(0, url.indexOf('?')), I don't see url_parts[0] used. Also are you saying that we have URLs w/ additional ?'s in them? It looks like line 22 assumes that all url_parts are in the form of "a=b" since it splits on all '=' characters (otherwise the test fails) I think most of this query parsing code is a little messed up (i.e., why aren't we splitting on '&' to grab individual parameters?) http://codereview.chromium.org/10647007/diff/13001/chrome/test/data/media/pla... chrome/test/data/media/player.html:29: container.innerHTML = '<' + tag + ' controls id="player"></' + tag + '>'; woah this is super gross mind cleaning this up to use DOM objects? var foo = document.createElement(tag); // etc... http://codereview.chromium.org/10647007/diff/13001/content/browser/media_brow... File content/browser/media_browsertest.cc (right): http://codereview.chromium.org/10647007/diff/13001/content/browser/media_brow... content/browser/media_browsertest.cc:75: RunVideoTest("bear.ogv"); Instead of running two tests in one these should be parameterized tests. For example it won't be immediately obvious whether the file:// or http:// case failed. You can use the IN_PROC_BROWSER_TEST_P() macro + INSTANTIATE_TEST_CASE_P() to parametrize tests. Take a look at: http://src.chromium.org/viewvc/chrome/trunk/src/chrome/test/perf/rendering/la... You can have MediaTest implement ::testing::WithParamInterface<T> where T can be of your choosing then use GetParam() to get the value.
I also noticed your try runs are timing out for the tests you've written/modified -- is that expected or is there something going on?
I also noticed your try runs are timing out for the tests you've written/modified -- is that expected or is there something going on?
On 2012/07/12 17:20:34, scherkus wrote: > I also noticed your try runs are timing out for the tests you've > written/modified -- is that expected or is there something going on? Not expected :-/ Even bear is taking > 45 seconds to play sometimes! Will investigate and fix things up. I suspect the new Tulip test cases might be failing due to: http://code.google.com/p/chromium/issues/detail?id=120749
http://codereview.chromium.org/10647007/diff/1/content/browser/media_browsert... File content/browser/media_browsertest.cc (right): http://codereview.chromium.org/10647007/diff/1/content/browser/media_browsert... content/browser/media_browsertest.cc:18: if (http) nit: {} http://codereview.chromium.org/10647007/diff/1/content/browser/media_browsert... content/browser/media_browsertest.cc:31: ASSERT_TRUE(test_server()->Start()); ASSERT just returns from the current function. So, you'll need to wrap calls to this in ASSERT_NO_FATAL_FAILURE up the chain. http://codereview.chromium.org/10647007/diff/13001/content/browser/media_brow... File content/browser/media_browsertest.cc (right): http://codereview.chromium.org/10647007/diff/13001/content/browser/media_brow... content/browser/media_browsertest.cc:31: if (http) You need to wrap all callers to this (and any other nested functions) in ASSERT_NO_FATAL_FAILURE
PTAL. This patch set also controversially (probably) adds --disable-audio to the renderer flags since we can't seem to count on the Buildbots/Trybots to have working audio ouput devices attached: http://code.google.com/p/chromium/issues/detail?id=120749 All tests seem to be stable and passing, whereas without the Tulip test ends in ERROR and video-loop never finishes. Obviously we should look into fixing 12749, but in the meantime --disable-audio should deflake these tests and add plenty of additional coverage. http://codereview.chromium.org/10647007/diff/1/content/browser/media_browsert... File content/browser/media_browsertest.cc (right): http://codereview.chromium.org/10647007/diff/1/content/browser/media_browsert... content/browser/media_browsertest.cc:18: if (http) On 2012/07/16 16:56:51, sky wrote: > nit: {} Will do, in next patch set. Forgot it for this try run. http://codereview.chromium.org/10647007/diff/1/content/browser/media_browsert... content/browser/media_browsertest.cc:31: ASSERT_TRUE(test_server()->Start()); On 2012/07/16 16:56:51, sky wrote: > ASSERT just returns from the current function. So, you'll need to wrap calls to > this in ASSERT_NO_FATAL_FAILURE up the chain. Done. http://codereview.chromium.org/10647007/diff/13001/chrome/test/data/media/pla... File chrome/test/data/media/player.html (right): http://codereview.chromium.org/10647007/diff/13001/chrome/test/data/media/pla... chrome/test/data/media/player.html:15: var url_parts = [url.substring(0, url.indexOf('?')), On 2012/07/12 17:19:56, scherkus wrote: > I don't see url_parts[0] used. Also are you saying that we have URLs w/ > additional ?'s in them? > > It looks like line 22 assumes that all url_parts are in the form of "a=b" since > it splits on all '=' characters (otherwise the test fails) > > I think most of this query parsing code is a little messed up (i.e., why aren't > we splitting on '&' to grab individual parameters?) The test passes audio=<file> and video=<file> so there's only one query parameter and the key doubles as the tag name. There were additional ?'s in the URL with wave generation; not anymore. Cleaned up a little bit. http://codereview.chromium.org/10647007/diff/13001/chrome/test/data/media/pla... chrome/test/data/media/player.html:29: container.innerHTML = '<' + tag + ' controls id="player"></' + tag + '>'; On 2012/07/12 17:19:56, scherkus wrote: > woah this is super gross > > mind cleaning this up to use DOM objects? > var foo = document.createElement(tag); > // etc... Done. http://codereview.chromium.org/10647007/diff/13001/content/browser/media_brow... File content/browser/media_browsertest.cc (right): http://codereview.chromium.org/10647007/diff/13001/content/browser/media_brow... content/browser/media_browsertest.cc:31: if (http) On 2012/07/16 16:56:51, sky wrote: > You need to wrap all callers to this (and any other nested functions) in > ASSERT_NO_FATAL_FAILURE Done, I think. not sure exactly which method you're referring to. I wrapped all of the PlayMedia() methods though. http://codereview.chromium.org/10647007/diff/13001/content/browser/media_brow... content/browser/media_browsertest.cc:75: RunVideoTest("bear.ogv"); On 2012/07/12 17:19:56, scherkus wrote: > Instead of running two tests in one these should be parameterized tests. For > example it won't be immediately obvious whether the file:// or http:// case > failed. > > You can use the IN_PROC_BROWSER_TEST_P() macro + INSTANTIATE_TEST_CASE_P() to > parametrize tests. Take a look at: > http://src.chromium.org/viewvc/chrome/trunk/src/chrome/test/perf/rendering/la... > > You can have MediaTest implement ::testing::WithParamInterface<T> where T can be > of your choosing then use GetParam() to get the value. 2 things: 1. I'm not sure this is possible now since ContentBrowserTest extends BrowserTestBase which automatically extends testing::Test. Trying to also extend testing::WithParam<> will be ambiguous. I don't see any other content tests using testing::WithParam, so I assume this is unsupported currently. 2. I'm not sure this is as clean as you think it will be. While, the code will certainly look nicer, test failures will now show up as "MediaTest.HttpPlaybackTest/1 failed" instead of "MediaTest.VideoBearTheora" failed. The only way to get the best of both worlds would be to have VideoBearTheoraHttp and VideoBearTheoraFile methods, which is a bit verbose in practice.
http://codereview.chromium.org/10647007/diff/28007/content/browser/media_brow... File content/browser/media_browsertest.cc (right): http://codereview.chromium.org/10647007/diff/28007/content/browser/media_brow... content/browser/media_browsertest.cc:18: class MediaTest : public content::ContentBrowserTest { Hmm... we should really avoid doing both file:// and http:// tests in a single test. FWIW ::testing::WithParamInterface<T> is not the same as ::testing::WithParam<T> -- the former can be used when extending an existing class that already inherits from ::testing::Test while the latter is essentially ::testing::Test+WithParam<T> Re: test names -- I was thinking of keeping the existing tests but using INSTANTIATE_TEST_CASE_P() like so: OPTION 1: INSTANTIATE_TEST_CASE_P(File, MediaTest, ::testing::Values(false)); INSTANTIATE_TEST_CASE_P(Http, MediaTest, ::testing::Values(true)); Produces the following test names: File/MediaTest.VideoBearTheora/0 Http/MediaTest.VideoBearTheora/0 OPTION 2 (Protocol is a crummy name): INSTANTIATE_TEST_CASE_P(Protocol, MediaTest, ::testing::Bool()); Produces the following test names: Protocol/MediaTest.VideoBearTheora/0 Protocol/MediaTest.VideoBearTheora/1 ...and we'd have to know that 0=file and 1=http or vice-versa http://codereview.chromium.org/10647007/diff/28007/content/browser/media_brow... content/browser/media_browsertest.cc:166: // Not all Buildbots have viable Audio devices, disable audio to prevent what's a viable audio device? also: s/Audio/audio/ also: TODO(dalecurtis) http://codereview.chromium.org/10647007/diff/28007/content/test/data/media/pl... File content/test/data/media/player.html (right): http://codereview.chromium.org/10647007/diff/28007/content/test/data/media/pl... content/test/data/media/player.html:20: if (url_parts.length == 2) { nit: early return instead of deep nesting if (url_parts.length != 2) { document.title = 'FAILED'; return; } Or use a helper function that returns a bool http://codereview.chromium.org/10647007/diff/28007/content/test/data/media/pl... content/test/data/media/player.html:22: if (query_parts.length == 2) { here too http://codereview.chromium.org/10647007/diff/28007/content/test/data/media/pl... content/test/data/media/player.html:25: if (tag == 'audio' || tag == 'video') { and here
http://codereview.chromium.org/10647007/diff/28007/content/browser/media_brow... File content/browser/media_browsertest.cc (right): http://codereview.chromium.org/10647007/diff/28007/content/browser/media_brow... content/browser/media_browsertest.cc:148: // ~17 seconds. file:// case is below, since the test needs to be split up to this seems bad. can't you achieve the same thing by playing the video at higher speed? i.e. using playbackRate ?
http://codereview.chromium.org/10647007/diff/28007/content/browser/media_brow... File content/browser/media_browsertest.cc (right): http://codereview.chromium.org/10647007/diff/28007/content/browser/media_brow... content/browser/media_browsertest.cc:18: class MediaTest : public content::ContentBrowserTest { On 2012/07/31 18:30:22, scherkus wrote: > Hmm... we should really avoid doing both file:// and http:// tests in a single > test. > > FWIW ::testing::WithParamInterface<T> is not the same as ::testing::WithParam<T> > -- the former can be used when extending an existing class that already inherits > from ::testing::Test while the latter is essentially > ::testing::Test+WithParam<T> > > Re: test names -- I was thinking of keeping the existing tests but using > INSTANTIATE_TEST_CASE_P() like so: > > OPTION 1: > INSTANTIATE_TEST_CASE_P(File, MediaTest, ::testing::Values(false)); > INSTANTIATE_TEST_CASE_P(Http, MediaTest, ::testing::Values(true)); > > Produces the following test names: > File/MediaTest.VideoBearTheora/0 > Http/MediaTest.VideoBearTheora/0 > > OPTION 2 (Protocol is a crummy name): > INSTANTIATE_TEST_CASE_P(Protocol, MediaTest, ::testing::Bool()); > > Produces the following test names: > Protocol/MediaTest.VideoBearTheora/0 > Protocol/MediaTest.VideoBearTheora/1 > > ...and we'd have to know that 0=file and 1=http or vice-versa Ah! I didn't know about WithParamInterface, I didn't even notice that's what you wrote in the first comment; sorry! I'll go with the File|Http/MediaTest.<TestName>/0 or see if I can find something that allows us to clean up the code too. http://codereview.chromium.org/10647007/diff/28007/content/browser/media_brow... content/browser/media_browsertest.cc:148: // ~17 seconds. file:// case is below, since the test needs to be split up to On 2012/07/31 18:41:06, John Abd-El-Malek wrote: > this seems bad. can't you achieve the same thing by playing the video at higher > speed? i.e. using playbackRate ? Well, all tests will be split if I go the route scherkus suggested above. We could use playback rate, but that's testing something slightly different. A high enough playback rate will end up in disabled audio too. Perhaps a better test might be a combination of playback rate and seeking? video.play(); onPlaying(setTimeout('video.playbackRate = 2', video.duration / 4)); onRateChange(setTimeout('video.playBackRate = 1;', video.duration / 4); onRateChange(video.currentTime = video.duration / 2, 0)); We can then set the final title to be the correct sequence of events instead of just the last event. This is similar to what we do for the PyAuto basic playback test. WDYT? http://codereview.chromium.org/10647007/diff/28007/content/browser/media_brow... content/browser/media_browsertest.cc:166: // Not all Buildbots have viable Audio devices, disable audio to prevent On 2012/07/31 18:30:22, scherkus wrote: > what's a viable audio device? > > also: s/Audio/audio/ > also: TODO(dalecurtis) I'll reword. By viable I meant a device which is not hardware muted, which prevents the output audio output buffer from ever draining.
http://codereview.chromium.org/10647007/diff/28007/content/browser/media_brow... File content/browser/media_browsertest.cc (right): http://codereview.chromium.org/10647007/diff/28007/content/browser/media_brow... content/browser/media_browsertest.cc:148: // ~17 seconds. file:// case is below, since the test needs to be split up to On 2012/07/31 19:33:33, DaleCurtis wrote: > On 2012/07/31 18:41:06, John Abd-El-Malek wrote: > > this seems bad. can't you achieve the same thing by playing the video at > higher > > speed? i.e. using playbackRate ? > > Well, all tests will be split if I go the route scherkus suggested above. We > could use playback rate, but that's testing something slightly different. I wasn't talking about "splitting", just about a test that takes 17 seconds to finish. That's really slow. Why does this need to be a 17s video? i.e. would the bug that this test is trying to cover need a video that long? Or will it also have reproduced with, say, a 1 (or 2, or 3 etc) second video over http? Whatever length of video that is needed to trigger this, would it also have triggered if you were using a playbackrate of like 5x? As you can see, I'm trying to figure out how we can get the same regression testing through a test that runs in the shortest amount of time. > > A high enough playback rate will end up in disabled audio too. Perhaps a better > test might be a combination of playback rate and seeking? > > video.play(); > onPlaying(setTimeout('video.playbackRate = 2', video.duration / 4)); > onRateChange(setTimeout('video.playBackRate = 1;', video.duration / 4); > onRateChange(video.currentTime = video.duration / 2, 0)); > > We can then set the final title to be the correct sequence of events instead of > just the last event. This is similar to what we do for the PyAuto basic playback > test. WDYT?
http://codereview.chromium.org/10647007/diff/28007/content/browser/media_brow... File content/browser/media_browsertest.cc (right): http://codereview.chromium.org/10647007/diff/28007/content/browser/media_brow... content/browser/media_browsertest.cc:148: // ~17 seconds. file:// case is below, since the test needs to be split up to On 2012/07/31 19:40:53, John Abd-El-Malek wrote: > On 2012/07/31 19:33:33, DaleCurtis wrote: > > On 2012/07/31 18:41:06, John Abd-El-Malek wrote: > > > this seems bad. can't you achieve the same thing by playing the video at > > higher > > > speed? i.e. using playbackRate ? > > > > Well, all tests will be split if I go the route scherkus suggested above. We > > could use playback rate, but that's testing something slightly different. > > I wasn't talking about "splitting", just about a test that takes 17 seconds to > finish. That's really slow. > > Why does this need to be a 17s video? i.e. would the bug that this test is > trying to cover need a video that long? Or will it also have reproduced with, > say, a 1 (or 2, or 3 etc) second video over http? > > Whatever length of video that is needed to trigger this, would it also have > triggered if you were using a playbackrate of like 5x? > > As you can see, I'm trying to figure out how we can get the same regression > testing through a test that runs in the shortest amount of time. > > > > > A high enough playback rate will end up in disabled audio too. Perhaps a > better > > test might be a combination of playback rate and seeking? > > > > video.play(); > > onPlaying(setTimeout('video.playbackRate = 2', video.duration / 4)); > > onRateChange(setTimeout('video.playBackRate = 1;', video.duration / 4); > > onRateChange(video.currentTime = video.duration / 2, 0)); > > > > We can then set the final title to be the correct sequence of events instead > of > > just the last event. This is similar to what we do for the PyAuto basic > playback > > test. WDYT? > The original bug needed > 4MB of media data to trigger consistently. The video in question happened to be ~ that large and free to use. The bug didn't occur immediately, but slightly after playback began. So, letting this video play for a few seconds and then seeking to the end should also expose the issue. Which is essentially what I was proposing above with the twist that we can get some extra coverage of playback rate in there. We know our playback rate adjustment code is a bit wacky for high values, so I'm not sure I'd want to rely on it for our basic tests.
On 2012/07/31 19:55:19, DaleCurtis wrote: > http://codereview.chromium.org/10647007/diff/28007/content/browser/media_brow... > File content/browser/media_browsertest.cc (right): > > http://codereview.chromium.org/10647007/diff/28007/content/browser/media_brow... > content/browser/media_browsertest.cc:148: // ~17 seconds. file:// case is > below, since the test needs to be split up to > On 2012/07/31 19:40:53, John Abd-El-Malek wrote: > > On 2012/07/31 19:33:33, DaleCurtis wrote: > > > On 2012/07/31 18:41:06, John Abd-El-Malek wrote: > > > > this seems bad. can't you achieve the same thing by playing the video at > > > higher > > > > speed? i.e. using playbackRate ? > > > > > > Well, all tests will be split if I go the route scherkus suggested above. > We > > > could use playback rate, but that's testing something slightly different. > > > > I wasn't talking about "splitting", just about a test that takes 17 seconds to > > finish. That's really slow. > > > > Why does this need to be a 17s video? i.e. would the bug that this test is > > trying to cover need a video that long? Or will it also have reproduced with, > > say, a 1 (or 2, or 3 etc) second video over http? > > > > Whatever length of video that is needed to trigger this, would it also have > > triggered if you were using a playbackrate of like 5x? > > > > As you can see, I'm trying to figure out how we can get the same regression > > testing through a test that runs in the shortest amount of time. > > > > > > > > A high enough playback rate will end up in disabled audio too. Perhaps a > > better > > > test might be a combination of playback rate and seeking? > > > > > > video.play(); > > > onPlaying(setTimeout('video.playbackRate = 2', video.duration / 4)); > > > onRateChange(setTimeout('video.playBackRate = 1;', video.duration / 4); > > > onRateChange(video.currentTime = video.duration / 2, 0)); > > > > > > We can then set the final title to be the correct sequence of events instead > > of > > > just the last event. This is similar to what we do for the PyAuto basic > > playback > > > test. WDYT? > > > > The original bug needed > 4MB of media data to trigger consistently. The video > in question happened to be ~ that large and free to use. The bug didn't occur > immediately, but slightly after playback began. So, letting this video play for > a few seconds and then seeking to the end should also expose the issue. > > Which is essentially what I was proposing above with the twist that we can get > some extra coverage of playback rate in there. OK, so if you check with the code before the fix for the bug, can you make a test that runs in 1-2 seconds and fails consistently, and passes consistently after the fix? > > We know our playback rate adjustment code is a bit wacky for high values, so I'm > not sure I'd want to rely on it for our basic tests. What do you mean by wacky?
On 2012/07/31 20:55:26, John Abd-El-Malek wrote: > On 2012/07/31 19:55:19, DaleCurtis wrote: > > > http://codereview.chromium.org/10647007/diff/28007/content/browser/media_brow... > > File content/browser/media_browsertest.cc (right): > > > > > http://codereview.chromium.org/10647007/diff/28007/content/browser/media_brow... > > content/browser/media_browsertest.cc:148: // ~17 seconds. file:// case is > > below, since the test needs to be split up to > > On 2012/07/31 19:40:53, John Abd-El-Malek wrote: > > > On 2012/07/31 19:33:33, DaleCurtis wrote: > > > > On 2012/07/31 18:41:06, John Abd-El-Malek wrote: > > > > > this seems bad. can't you achieve the same thing by playing the video at > > > > higher > > > > > speed? i.e. using playbackRate ? > > > > > > > > Well, all tests will be split if I go the route scherkus suggested above. > > We > > > > could use playback rate, but that's testing something slightly different. > > > > > > I wasn't talking about "splitting", just about a test that takes 17 seconds > to > > > finish. That's really slow. > > > > > > Why does this need to be a 17s video? i.e. would the bug that this test is > > > trying to cover need a video that long? Or will it also have reproduced > with, > > > say, a 1 (or 2, or 3 etc) second video over http? > > > > > > Whatever length of video that is needed to trigger this, would it also have > > > triggered if you were using a playbackrate of like 5x? > > > > > > As you can see, I'm trying to figure out how we can get the same regression > > > testing through a test that runs in the shortest amount of time. > > > > > > > > > > > A high enough playback rate will end up in disabled audio too. Perhaps a > > > better > > > > test might be a combination of playback rate and seeking? > > > > > > > > video.play(); > > > > onPlaying(setTimeout('video.playbackRate = 2', video.duration / 4)); > > > > onRateChange(setTimeout('video.playBackRate = 1;', video.duration / 4); > > > > onRateChange(video.currentTime = video.duration / 2, 0)); > > > > > > > > We can then set the final title to be the correct sequence of events > instead > > > of > > > > just the last event. This is similar to what we do for the PyAuto basic > > > playback > > > > test. WDYT? > > > > > > > The original bug needed > 4MB of media data to trigger consistently. The video > > in question happened to be ~ that large and free to use. The bug didn't occur > > immediately, but slightly after playback began. So, letting this video play > for > > a few seconds and then seeking to the end should also expose the issue. > > > > Which is essentially what I was proposing above with the twist that we can get > > some extra coverage of playback rate in there. > > OK, so if you check with the code before the fix for the bug, can you make a > test that runs in 1-2 seconds and fails consistently, and passes consistently > after the fix? It takes 1-2 seconds just to spin up and tear down the whole thing :), so that might be a little too low. I implemented the seek test I mentioned above and test completion takes ~7.4seconds. It's been long enough now that rolling back to the original bug will be difficult, but I'll see what I can do. > > > > > We know our playback rate adjustment code is a bit wacky for high values, so > I'm > > not sure I'd want to rely on it for our basic tests. > > What do you mean by wacky? Ended event comes late, audio glitches, audio disabled with certain playback rates, etc, etc. In general it works well enough, but we know it needs an overhaul: http://code.google.com/p/chromium/issues/list?can=2&q=playback+rate+feature%3...
On Tue, Jul 31, 2012 at 2:24 PM, <dalecurtis@chromium.org> wrote: > On 2012/07/31 20:55:26, John Abd-El-Malek wrote: > >> On 2012/07/31 19:55:19, DaleCurtis wrote: >> > >> > > http://codereview.chromium.**org/10647007/diff/28007/** > content/browser/media_**browsertest.cc<http://codereview.chromium.org/10647007/diff/28007/content/browser/media_browsertest.cc> > >> > File content/browser/media_**browsertest.cc (right): >> > >> > >> > > http://codereview.chromium.**org/10647007/diff/28007/** > content/browser/media_**browsertest.cc#newcode148<http://codereview.chromium.org/10647007/diff/28007/content/browser/media_browsertest.cc#newcode148> > >> > content/browser/media_**browsertest.cc:148: // ~17 seconds. file:// >> case is >> > below, since the test needs to be split up to >> > On 2012/07/31 19:40:53, John Abd-El-Malek wrote: >> > > On 2012/07/31 19:33:33, DaleCurtis wrote: >> > > > On 2012/07/31 18:41:06, John Abd-El-Malek wrote: >> > > > > this seems bad. can't you achieve the same thing by playing the >> video >> > at > >> > > > higher >> > > > > speed? i.e. using playbackRate ? >> > > > >> > > > Well, all tests will be split if I go the route scherkus suggested >> > above. > >> > We >> > > > could use playback rate, but that's testing something slightly >> > different. > >> > > >> > > I wasn't talking about "splitting", just about a test that takes 17 >> > seconds > >> to >> > > finish. That's really slow. >> > > >> > > Why does this need to be a 17s video? i.e. would the bug that this >> test is >> > > trying to cover need a video that long? Or will it also have >> reproduced >> with, >> > > say, a 1 (or 2, or 3 etc) second video over http? >> > > >> > > Whatever length of video that is needed to trigger this, would it also >> > have > >> > > triggered if you were using a playbackrate of like 5x? >> > > >> > > As you can see, I'm trying to figure out how we can get the same >> > regression > >> > > testing through a test that runs in the shortest amount of time. >> > > >> > > > >> > > > A high enough playback rate will end up in disabled audio too. >> Perhaps >> > a > >> > > better >> > > > test might be a combination of playback rate and seeking? >> > > > >> > > > video.play(); >> > > > onPlaying(setTimeout('video.**playbackRate = 2', video.duration / >> 4)); >> > > > onRateChange(setTimeout('**video.playBackRate = 1;', >> video.duration / 4); >> > > > onRateChange(video.currentTime = video.duration / 2, 0)); >> > > > >> > > > We can then set the final title to be the correct sequence of events >> instead >> > > of >> > > > just the last event. This is similar to what we do for the PyAuto >> basic >> > > playback >> > > > test. WDYT? >> > > >> > >> > The original bug needed > 4MB of media data to trigger consistently. The >> > video > >> > in question happened to be ~ that large and free to use. The bug didn't >> > occur > >> > immediately, but slightly after playback began. So, letting this video >> play >> for >> > a few seconds and then seeking to the end should also expose the issue. >> > >> > Which is essentially what I was proposing above with the twist that we >> can >> > get > >> > some extra coverage of playback rate in there. >> > > OK, so if you check with the code before the fix for the bug, can you >> make a >> test that runs in 1-2 seconds and fails consistently, and passes >> consistently >> after the fix? >> > > It takes 1-2 seconds just to spin up and tear down the whole thing :), so > that > might be a little too low. I implemented the seek test I mentioned above > and > test completion takes ~7.4seconds. > > It's been long enough now that rolling back to the original bug will be > difficult, but I'll see what I can do. I was unclear; I really meant 1-2s of playback :) > > > > > >> > We know our playback rate adjustment code is a bit wacky for high >> values, so >> I'm >> > not sure I'd want to rely on it for our basic tests. >> > > What do you mean by wacky? >> > > Ended event comes late, audio glitches, audio disabled with certain > playback > rates, etc, etc. In general it works well enough, but we know it needs an > overhaul: > http://code.google.com/p/**chromium/issues/list?can=2&q=** > playback+rate+feature%3AMedia<http://code.google.com/p/chromium/issues/list?can=2&q=playback+rate+feature%3AMedia> > > http://codereview.chromium.**org/10647007/<http://codereview.chromium.org/106... >
On 2012/07/31 21:24:28, DaleCurtis wrote: > On 2012/07/31 20:55:26, John Abd-El-Malek wrote: > > On 2012/07/31 19:55:19, DaleCurtis wrote: > > > > > > http://codereview.chromium.org/10647007/diff/28007/content/browser/media_brow... > > > File content/browser/media_browsertest.cc (right): > > > > > > > > > http://codereview.chromium.org/10647007/diff/28007/content/browser/media_brow... > > > content/browser/media_browsertest.cc:148: // ~17 seconds. file:// case is > > > below, since the test needs to be split up to > > > On 2012/07/31 19:40:53, John Abd-El-Malek wrote: > > > > On 2012/07/31 19:33:33, DaleCurtis wrote: > > > > > On 2012/07/31 18:41:06, John Abd-El-Malek wrote: > > > > > > this seems bad. can't you achieve the same thing by playing the video > at > > > > > higher > > > > > > speed? i.e. using playbackRate ? > > > > > > > > > > Well, all tests will be split if I go the route scherkus suggested > above. > > > We > > > > > could use playback rate, but that's testing something slightly > different. > > > > > > > > I wasn't talking about "splitting", just about a test that takes 17 > seconds > > to > > > > finish. That's really slow. > > > > > > > > Why does this need to be a 17s video? i.e. would the bug that this test is > > > > trying to cover need a video that long? Or will it also have reproduced > > with, > > > > say, a 1 (or 2, or 3 etc) second video over http? > > > > > > > > Whatever length of video that is needed to trigger this, would it also > have > > > > triggered if you were using a playbackrate of like 5x? > > > > > > > > As you can see, I'm trying to figure out how we can get the same > regression > > > > testing through a test that runs in the shortest amount of time. > > > > > > > > > > > > > > A high enough playback rate will end up in disabled audio too. Perhaps > a > > > > better > > > > > test might be a combination of playback rate and seeking? > > > > > > > > > > video.play(); > > > > > onPlaying(setTimeout('video.playbackRate = 2', video.duration / 4)); > > > > > onRateChange(setTimeout('video.playBackRate = 1;', video.duration / 4); > > > > > onRateChange(video.currentTime = video.duration / 2, 0)); > > > > > > > > > > We can then set the final title to be the correct sequence of events > > instead > > > > of > > > > > just the last event. This is similar to what we do for the PyAuto basic > > > > playback > > > > > test. WDYT? > > > > > > > > > > The original bug needed > 4MB of media data to trigger consistently. The > video > > > in question happened to be ~ that large and free to use. The bug didn't > occur > > > immediately, but slightly after playback began. So, letting this video play > > for > > > a few seconds and then seeking to the end should also expose the issue. > > > > > > Which is essentially what I was proposing above with the twist that we can > get > > > some extra coverage of playback rate in there. > > > > OK, so if you check with the code before the fix for the bug, can you make a > > test that runs in 1-2 seconds and fails consistently, and passes consistently > > after the fix? > > It takes 1-2 seconds just to spin up and tear down the whole thing :), so that > might be a little too low. I implemented the seek test I mentioned above and > test completion takes ~7.4seconds. > > It's been long enough now that rolling back to the original bug will be > difficult, but I'll see what I can do. > > > > > > > > > We know our playback rate adjustment code is a bit wacky for high values, so > > I'm > > > not sure I'd want to rely on it for our basic tests. > > > > What do you mean by wacky? > > Ended event comes late, audio glitches, audio disabled with certain playback > rates, etc, etc. In general it works well enough, but we know it needs an > overhaul: > http://code.google.com/p/chromium/issues/list?can=2&q=playback+rate+feature%2... Since we're running w/ disabled audio using playback rate should be OK for tests of these sorts. We might drop frames and what not but it will speed things up. One caveat for HTTP tests is that we will buffer "more" based on the playback rate. i.e., if it's 2x we'll try to maintain a read-ahead buffer that's 2x more (up to a limit)
On 2012/07/31 21:40:35, scherkus wrote: > On 2012/07/31 21:24:28, DaleCurtis wrote: > > On 2012/07/31 20:55:26, John Abd-El-Malek wrote: > > > On 2012/07/31 19:55:19, DaleCurtis wrote: > > > > > > > > > > http://codereview.chromium.org/10647007/diff/28007/content/browser/media_brow... > > > > File content/browser/media_browsertest.cc (right): > > > > > > > > > > > > > > http://codereview.chromium.org/10647007/diff/28007/content/browser/media_brow... > > > > content/browser/media_browsertest.cc:148: // ~17 seconds. file:// case is > > > > below, since the test needs to be split up to > > > > On 2012/07/31 19:40:53, John Abd-El-Malek wrote: > > > > > On 2012/07/31 19:33:33, DaleCurtis wrote: > > > > > > On 2012/07/31 18:41:06, John Abd-El-Malek wrote: > > > > > > > this seems bad. can't you achieve the same thing by playing the > video > > at > > > > > > higher > > > > > > > speed? i.e. using playbackRate ? > > > > > > > > > > > > Well, all tests will be split if I go the route scherkus suggested > > above. > > > > We > > > > > > could use playback rate, but that's testing something slightly > > different. > > > > > > > > > > I wasn't talking about "splitting", just about a test that takes 17 > > seconds > > > to > > > > > finish. That's really slow. > > > > > > > > > > Why does this need to be a 17s video? i.e. would the bug that this test > is > > > > > trying to cover need a video that long? Or will it also have reproduced > > > with, > > > > > say, a 1 (or 2, or 3 etc) second video over http? > > > > > > > > > > Whatever length of video that is needed to trigger this, would it also > > have > > > > > triggered if you were using a playbackrate of like 5x? > > > > > > > > > > As you can see, I'm trying to figure out how we can get the same > > regression > > > > > testing through a test that runs in the shortest amount of time. > > > > > > > > > > > > > > > > > A high enough playback rate will end up in disabled audio too. > Perhaps > > a > > > > > better > > > > > > test might be a combination of playback rate and seeking? > > > > > > > > > > > > video.play(); > > > > > > onPlaying(setTimeout('video.playbackRate = 2', video.duration / 4)); > > > > > > onRateChange(setTimeout('video.playBackRate = 1;', video.duration / > 4); > > > > > > onRateChange(video.currentTime = video.duration / 2, 0)); > > > > > > > > > > > > We can then set the final title to be the correct sequence of events > > > instead > > > > > of > > > > > > just the last event. This is similar to what we do for the PyAuto > basic > > > > > playback > > > > > > test. WDYT? > > > > > > > > > > > > > The original bug needed > 4MB of media data to trigger consistently. The > > video > > > > in question happened to be ~ that large and free to use. The bug didn't > > occur > > > > immediately, but slightly after playback began. So, letting this video > play > > > for > > > > a few seconds and then seeking to the end should also expose the issue. > > > > > > > > Which is essentially what I was proposing above with the twist that we can > > get > > > > some extra coverage of playback rate in there. > > > > > > OK, so if you check with the code before the fix for the bug, can you make a > > > test that runs in 1-2 seconds and fails consistently, and passes > consistently > > > after the fix? > > > > It takes 1-2 seconds just to spin up and tear down the whole thing :), so that > > might be a little too low. I implemented the seek test I mentioned above and > > test completion takes ~7.4seconds. > > > > It's been long enough now that rolling back to the original bug will be > > difficult, but I'll see what I can do. > > > > > > > > > > > > > We know our playback rate adjustment code is a bit wacky for high values, > so > > > I'm > > > > not sure I'd want to rely on it for our basic tests. > > > > > > What do you mean by wacky? > > > > Ended event comes late, audio glitches, audio disabled with certain playback > > rates, etc, etc. In general it works well enough, but we know it needs an > > overhaul: > > > http://code.google.com/p/chromium/issues/list?can=2&q=playback+rate+feature%2... > > Since we're running w/ disabled audio using playback rate should be OK for tests > of these sorts. We might drop frames and what not but it will speed things up. > > One caveat for HTTP tests is that we will buffer "more" based on the playback > rate. i.e., if it's 2x we'll try to maintain a read-ahead buffer that's 2x more > (up to a limit) I forgot about the buffering changes with playback rate. I suspect playback rate would have allowed this test to pass the original issue given that reproducibility was affected by timing and size. However rolling back in the original issue (~1.5 months ago now) and getting a build is proving seemingly impossible (via git checkout <hash>). Ideas? I've updated the current patch set which gets the test down to ~5.3seconds (measured at gtest, so including setup+teardown) w/ 2 sec playback cap and a seek to 90% afterward (~3.7 seconds of playback). Is that sufficiently short? I'd like to avoid using playback rate if we can.
http://codereview.chromium.org/10647007/diff/29009/content/test/data/media/pl... File content/test/data/media/player.html (right): http://codereview.chromium.org/10647007/diff/29009/content/test/data/media/pl... content/test/data/media/player.html:37: function SeekTestTimeoutSetup() { setTimeout/Interval are sources of flakiness / non-determinism on loaded bots so I would recommend listening to timeupdate then issue a seek when playback time goes past a certain point
http://codereview.chromium.org/10647007/diff/29009/content/test/data/media/pl... File content/test/data/media/player.html (right): http://codereview.chromium.org/10647007/diff/29009/content/test/data/media/pl... content/test/data/media/player.html:37: function SeekTestTimeoutSetup() { On 2012/08/02 17:55:36, scherkus wrote: > setTimeout/Interval are sources of flakiness / non-determinism on loaded bots so > I would recommend listening to timeupdate then issue a seek when playback time > goes past a certain point Done.
LGTM w/ nit http://codereview.chromium.org/10647007/diff/23010/content/test/data/media/pl... File content/test/data/media/player.html (right): http://codereview.chromium.org/10647007/diff/23010/content/test/data/media/pl... content/test/data/media/player.html:64: player.addEventListener('ended', SeekTestStep, false); shouldn't this end the test if it executes first as opposed to seeking to 0.9*duration?
jam, sky: Any more comments? http://codereview.chromium.org/10647007/diff/23010/content/test/data/media/pl... File content/test/data/media/player.html (right): http://codereview.chromium.org/10647007/diff/23010/content/test/data/media/pl... content/test/data/media/player.html:64: player.addEventListener('ended', SeekTestStep, false); On 2012/08/02 20:02:08, scherkus wrote: > shouldn't this end the test if it executes first as opposed to seeking to > 0.9*duration? I did this intentionally to add seek coverage for all tests.
http://codereview.chromium.org/10647007/diff/23010/content/test/data/media/pl... File content/test/data/media/player.html (right): http://codereview.chromium.org/10647007/diff/23010/content/test/data/media/pl... content/test/data/media/player.html:64: player.addEventListener('ended', SeekTestStep, false); On 2012/08/02 20:56:10, DaleCurtis wrote: > On 2012/08/02 20:02:08, scherkus wrote: > > shouldn't this end the test if it executes first as opposed to seeking to > > 0.9*duration? > > I did this intentionally to add seek coverage for all tests. OK -- I wasn't sure if it was intentional or not -- how about updating the docs for RunTest() seeing as it has no mention of the seek test?
http://codereview.chromium.org/10647007/diff/23010/content/test/data/media/pl... File content/test/data/media/player.html (right): http://codereview.chromium.org/10647007/diff/23010/content/test/data/media/pl... content/test/data/media/player.html:64: player.addEventListener('ended', SeekTestStep, false); On 2012/08/02 21:33:31, scherkus wrote: > On 2012/08/02 20:56:10, DaleCurtis wrote: > > On 2012/08/02 20:02:08, scherkus wrote: > > > shouldn't this end the test if it executes first as opposed to seeking to > > > 0.9*duration? > > > > I did this intentionally to add seek coverage for all tests. > > OK -- I wasn't sure if it was intentional or not -- how about updating the docs > for RunTest() seeing as it has no mention of the seek test? I documented it on MediaTest, but forgot to update the text here. Done.
LGTM http://codereview.chromium.org/10647007/diff/36003/content/browser/media_brow... File content/browser/media_browsertest.cc (right): http://codereview.chromium.org/10647007/diff/36003/content/browser/media_brow... content/browser/media_browsertest.cc:37: virtual void SetUpCommandLine(CommandLine* command_line) { OVERRIDE http://codereview.chromium.org/10647007/diff/36003/content/browser/media_brow... content/browser/media_browsertest.cc:157: virtual void SetUpCommandLine(CommandLine* command_line) { OVERRIDE
jam: Anything else? http://codereview.chromium.org/10647007/diff/36003/content/browser/media_brow... File content/browser/media_browsertest.cc (right): http://codereview.chromium.org/10647007/diff/36003/content/browser/media_brow... content/browser/media_browsertest.cc:37: virtual void SetUpCommandLine(CommandLine* command_line) { On 2012/08/03 16:51:39, sky wrote: > OVERRIDE Done. http://codereview.chromium.org/10647007/diff/36003/content/browser/media_brow... content/browser/media_browsertest.cc:157: virtual void SetUpCommandLine(CommandLine* command_line) { On 2012/08/03 16:51:39, sky wrote: > OVERRIDE Done.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/10647007/24012
Failed to apply patch for content/browser/media_browsertest.cc: While running patch -p1 --forward --force; patching file content/browser/media_browsertest.cc Hunk #1 FAILED at 3. Hunk #2 succeeded at 124 (offset 4 lines). Hunk #3 succeeded at 156 (offset 4 lines). 1 out of 3 hunks FAILED -- saving rejects to file content/browser/media_browsertest.cc.rej
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/10647007/33009
Change committed as 150208 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
