72758 2011-11-18 14:03:18 -0800 Use canplay instead of canplaythrough in http/tests/media to prevent timeout 2011-11-28 10:40:49 -0800 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 vrk vrk acolwell adamk eric.carlson jamesr scherkus webkit.review.bot oldest_to_newest 506043 0 vrk 2011-11-18 14:03:18 -0800 Use canplay instead of canplaythrough in http/tests/media to prevent timeout 506044 1 115870 vrk 2011-11-18 14:03:53 -0800 Created attachment 115870 Patch 506066 2 eric.carlson 2011-11-18 14:33:56 -0800 Why does 'canplaythough' cause these tests to time out? 506107 3 vrk 2011-11-18 15:13:24 -0800 (In reply to comment #2) > Why does 'canplaythough' cause these tests to time out? In my 1st stab implementation of canplaythrough for chromium (that landed today and so far hasn't been reverted! http://codereview.chromium.org/8399023/), here is my main criteria for firing canplaythrough: a) Sample incoming downloading for 5 seconds and calculate download rate per second. If download rate >= media bitrate fire canplaythrough. b) But if we download 4 seconds of data in less 2 seconds (i.e. downloading least 2x faster than consuming), fire canplaythrough without waiting for the 5s window c) If we've buffered to the end of the file and still haven't fired canplaythrough, fire canplaythrough. This also means fully loaded source (e.g. file:// protocol) fires canplaythrough immediately. Running the tests from my local machine, the http test serves video data fast enough to trigger scenario (b). But it looks like on our windows bots, scenario (a) or (c) is happening, which is causing the test to time out. It doesn't appear checking for canplaythrough is needed for these tests, so I changed the event to canplay, an event that should trigger much earlier and more predictably. (Of course we shouldn't change the test to potentially cover up other bugs, though!) 506116 4 adamk 2011-11-18 15:18:56 -0800 Note that at least one other tests saw some behavior differences due to this change in canplaythrough: compositor/geometry/clipped-video-controller.html In that case, it wasn't a timeout, but a difference in layout (scrollHeight 293 vs 292) 506247 5 eric.carlson 2011-11-18 17:53:28 -0800 (In reply to comment #4) > Note that at least one other tests saw some behavior differences due to this change in canplaythrough: > > compositor/geometry/clipped-video-controller.html > > In that case, it wasn't a timeout, but a difference in layout (scrollHeight 293 vs 292) I can't imagine why the intrinsic size of the element would change by one pixel. This sounds like a bug, has anyone checked to see what is happening? 506257 6 eric.carlson 2011-11-18 18:01:18 -0800 (In reply to comment #3) > Running the tests from my local machine, the http test serves video data fast enough to trigger scenario (b). But it looks like on our windows bots, scenario (a) or (c) is happening, which is causing the test to time out. > > It doesn't appear checking for canplaythrough is needed for these tests, so I changed the event to canplay, an event that should trigger much earlier and more predictably. (Of course we shouldn't change the test to potentially cover up other bugs, though!) I agree that having these tests trigger on 'canplay' instead of 'canplaythough' won't change them, but I worry that you are papering over a problem in your new implementation because I don't see how a file loaded from a same-machine web server could load slowly enough to not trigger your logic. 506262 7 adamk 2011-11-18 18:08:24 -0800 (In reply to comment #5) > (In reply to comment #4) > > Note that at least one other tests saw some behavior differences due to this change in canplaythrough: > > > > compositor/geometry/clipped-video-controller.html > > > > In that case, it wasn't a timeout, but a difference in layout (scrollHeight 293 vs 292) > > I can't imagine why the intrinsic size of the element would change by one pixel. This sounds like a bug, has anyone checked to see what is happening? James was the one who suggested this on IRC, I'll leave the explanation to him. 507079 8 115870 webkit.review.bot 2011-11-21 11:00:12 -0800 Comment on attachment 115870 Patch Clearing flags on attachment: 115870 Committed r100936: <http://trac.webkit.org/changeset/100936> 507080 9 webkit.review.bot 2011-11-21 11:00:16 -0800 All reviewed patches have been landed. Closing bug. 508618 10 eric.carlson 2011-11-23 22:02:04 -0800 (In reply to comment #7) > (In reply to comment #5) > > > > I can't imagine why the intrinsic size of the element would change by one pixel. This sounds like a bug, has anyone checked to see what is happening? > > James was the one who suggested this on IRC, I'll leave the explanation to him. https://bugs.webkit.org/show_bug.cgi?id=72903 is also about a 1px change when a test is triggered by 'canplaythrough' instead of 'canplay'. In that bug James says: I don't feel qualified enough on these events to know what the change does. Can someone familiar with when these events fire and what might change the render tree please chime in with a theory of how this could help? and Victoria says: I don't know how this could cause a 1px rendering difference but the patch was landed anyway - as this one was. Does anyone know what caused the rendering change? 509942 11 adamk 2011-11-28 10:40:49 -0800 (In reply to comment #10) > (In reply to comment #7) > > (In reply to comment #5) > > > > > > I can't imagine why the intrinsic size of the element would change by one pixel. This sounds like a bug, has anyone checked to see what is happening? > > > > James was the one who suggested this on IRC, I'll leave the explanation to him. > > https://bugs.webkit.org/show_bug.cgi?id=72903 is also about a 1px change when a test is triggered by 'canplaythrough' instead of 'canplay'. In that bug James says: > > I don't feel qualified enough on these events to know what the > change does. Can someone familiar with when these events fire > and what might change the render tree please chime in with a > theory of how this could help? > > and Victoria says: > > I don't know how this could cause a 1px rendering difference > > but the patch was landed anyway - as this one was. Note that the patch on 72903 ended up being a better-understood workaround: the pixel results weren't flaky, only the text layout results, and so we suppressed the latter (as James pointed out, we only cared about the pixel output for that test anyway). > Does anyone know what caused the rendering change? But your point here still stands, the root cause of the difference in rendering has yet to be determined. 115870 2011-11-18 14:03:53 -0800 2011-11-21 11:00:12 -0800 Patch bug-72758-20111118140352.patch text/plain 3080 vrk U3VidmVyc2lvbiBSZXZpc2lvbjogMTAwODAyCmRpZmYgLS1naXQgYS9MYXlvdXRUZXN0cy9DaGFu Z2VMb2cgYi9MYXlvdXRUZXN0cy9DaGFuZ2VMb2cKaW5kZXggYzk4NjJmYjVmNWY1ODI4ODE4MWUz NjE5ZDg0YzM2MWVkMjE4NGEyYi4uY2JhMTY2MjRiZDllYzgzMWYyNGZjZmRiOWNjMDM4MzM5ZTdh MDlmNyAxMDA3NTUKLS0tIGEvTGF5b3V0VGVzdHMvQ2hhbmdlTG9nCisrKyBiL0xheW91dFRlc3Rz L0NoYW5nZUxvZwpAQCAtMSwzICsxLDE1IEBACisyMDExLTExLTE4ICBWaWN0b3JpYSBLaXJzdCAg PHZya0BjaHJvbWl1bS5vcmc+CisKKyAgICAgICAgVXNlIGNhbnBsYXkgaW5zdGVhZCBvZiBjYW5w bGF5dGhyb3VnaCBpbiBodHRwL3Rlc3RzL21lZGlhIHRvIHByZXZlbnQgdGltZW91dAorICAgICAg ICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9NzI3NTgKKworICAgICAg ICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICAqIGh0dHAvdGVzdHMvbWVk aWEvdmlkZW8tY29va2llLWV4cGVjdGVkLnR4dDoKKyAgICAgICAgKiBodHRwL3Rlc3RzL21lZGlh L3ZpZGVvLWNvb2tpZS5odG1sOgorICAgICAgICAqIGh0dHAvdGVzdHMvbWVkaWEvdmlkZW8tcmVm ZXJlci1leHBlY3RlZC50eHQ6CisgICAgICAgICogaHR0cC90ZXN0cy9tZWRpYS92aWRlby1yZWZl cmVyLmh0bWw6CisKIDIwMTEtMTEtMTggIEFkYW0gS2xlaW4gIDxhZGFta0BjaHJvbWl1bS5vcmc+ CiAKICAgICAgICAgUmVtb3ZlIElNQUdFIHN1cHByZXNzaW9uIGZvciBjc3MzL2ltYWdlcy9jcm9z cy1mYWRlLW92ZXJmbG93LXBvc2l0aW9uLmh0bWwKZGlmZiAtLWdpdCBhL0xheW91dFRlc3RzL2h0 dHAvdGVzdHMvbWVkaWEvdmlkZW8tY29va2llLWV4cGVjdGVkLnR4dCBiL0xheW91dFRlc3RzL2h0 dHAvdGVzdHMvbWVkaWEvdmlkZW8tY29va2llLWV4cGVjdGVkLnR4dAppbmRleCAxNWY5MWQyMzM3 YzQxMmE5NjEzMWIxODRjMjMxOTg2NmUwZjIzZjZhLi5hNzg3NjkyNDk3ZjcyZjQ0Zjc3NjQyMzRm Y2VjZDhkNWRmMDM0MDkyIDEwMDY0NAotLS0gYS9MYXlvdXRUZXN0cy9odHRwL3Rlc3RzL21lZGlh L3ZpZGVvLWNvb2tpZS1leHBlY3RlZC50eHQKKysrIGIvTGF5b3V0VGVzdHMvaHR0cC90ZXN0cy9t ZWRpYS92aWRlby1jb29raWUtZXhwZWN0ZWQudHh0CkBAIC0xLDMgKzEsMyBAQAotRVZFTlQoY2Fu cGxheXRocm91Z2gpCitFVkVOVChjYW5wbGF5KQogVGVzdHMgdGhhdCB0aGUgbWVkaWEgcGxheWVy IHdpbGwgc2VuZCB0aGUgcmVsZXZhbnQgY29va2llcyB3aGVuIHJlcXVlc3RpbmcgdGhlIG1lZGlh IGZpbGUuCiAKZGlmZiAtLWdpdCBhL0xheW91dFRlc3RzL2h0dHAvdGVzdHMvbWVkaWEvdmlkZW8t Y29va2llLmh0bWwgYi9MYXlvdXRUZXN0cy9odHRwL3Rlc3RzL21lZGlhL3ZpZGVvLWNvb2tpZS5o dG1sCmluZGV4IDA2ZGVmYTIyZDU3MWMwMWM0MmVlYjM1YWU3ZjZjZmM1NDgxZTRkYmMuLjhlZTRh MmVlYTQyNGEzODBlYTVkMzY1NGYzMGE1NDJkYTU1ZDc5ZTUgMTAwNjQ0Ci0tLSBhL0xheW91dFRl c3RzL2h0dHAvdGVzdHMvbWVkaWEvdmlkZW8tY29va2llLmh0bWwKKysrIGIvTGF5b3V0VGVzdHMv aHR0cC90ZXN0cy9tZWRpYS92aWRlby1jb29raWUuaHRtbApAQCAtMjgsNyArMjgsNyBAQAogICAg ICAgICBmcmFtZS5zcmMgPSAiaHR0cDovLzEyNy4wLjAuMTo4MDAwL21lZGlhL3Jlc291cmNlcy9z ZXRDb29raWUuY2dpP25hbWU9IiArIG1vdmllOwogICAgIH0KIAotICAgIHdhaXRGb3JFdmVudCgi Y2FucGxheXRocm91Z2giLCBmdW5jdGlvbiAoKSB7CisgICAgd2FpdEZvckV2ZW50KCJjYW5wbGF5 IiwgZnVuY3Rpb24gKCkgewogICAgICAgICBpZiAod2luZG93LmxheW91dFRlc3RDb250cm9sbGVy KQogICAgICAgICAgICAgd2luZG93LmxheW91dFRlc3RDb250cm9sbGVyLm5vdGlmeURvbmUoKTsK ICAgICB9ICk7CmRpZmYgLS1naXQgYS9MYXlvdXRUZXN0cy9odHRwL3Rlc3RzL21lZGlhL3ZpZGVv LXJlZmVyZXItZXhwZWN0ZWQudHh0IGIvTGF5b3V0VGVzdHMvaHR0cC90ZXN0cy9tZWRpYS92aWRl by1yZWZlcmVyLWV4cGVjdGVkLnR4dAppbmRleCA0ODZlNTYzNWRiNjFiNTRiZGYwOTk5ZDQyYWIy OWE2NWU3YjY0YzZjLi41NzRhOGY4MzcwYzU3ZGIwZmU2ZjUzYTUzYWEwNmM2OGUwNmUwOTA4IDEw MDY0NAotLS0gYS9MYXlvdXRUZXN0cy9odHRwL3Rlc3RzL21lZGlhL3ZpZGVvLXJlZmVyZXItZXhw ZWN0ZWQudHh0CisrKyBiL0xheW91dFRlc3RzL2h0dHAvdGVzdHMvbWVkaWEvdmlkZW8tcmVmZXJl ci1leHBlY3RlZC50eHQKQEAgLTEsMyArMSwzIEBACi1FVkVOVChjYW5wbGF5dGhyb3VnaCkKK0VW RU5UKGNhbnBsYXkpCiBUZXN0cyB0aGF0IHRoZSBtZWRpYSBwbGF5ZXIgd2lsbCBzZW5kIHRoZSBy ZWxldmFudCByZWZlcmVyIHdoZW4gcmVxdWVzdGluZyB0aGUgbWVkaWEgZmlsZS4KIApkaWZmIC0t Z2l0IGEvTGF5b3V0VGVzdHMvaHR0cC90ZXN0cy9tZWRpYS92aWRlby1yZWZlcmVyLmh0bWwgYi9M YXlvdXRUZXN0cy9odHRwL3Rlc3RzL21lZGlhL3ZpZGVvLXJlZmVyZXIuaHRtbAppbmRleCBlNmU5 Zjc5ZGE4ZmUzY2RhZmU1MzZlYTJhOTVjYWJkNWFjZjViOGVmLi44Mzc3MGEwNzYxMTc3YmNiYWFh ZTA1OTc1YjdhNzUyNzUxNDVkMWQxIDEwMDY0NAotLS0gYS9MYXlvdXRUZXN0cy9odHRwL3Rlc3Rz L21lZGlhL3ZpZGVvLXJlZmVyZXIuaHRtbAorKysgYi9MYXlvdXRUZXN0cy9odHRwL3Rlc3RzL21l ZGlhL3ZpZGVvLXJlZmVyZXIuaHRtbApAQCAtMzAsNyArMzAsNyBAQAogICAgICAgICBjb25zb2xl V3JpdGUoJ0ZBSUwsIGdvdCBlcnJvciB3aGVuIGxvYWRpbmcgbWVkaWEgZXJyb3I6ICcgKyB2aWRl by5lcnJvci5jb2RlKTsKICAgICB9KTsgICAgCiAKLSAgICB3YWl0Rm9yRXZlbnQoJ2NhbnBsYXl0 aHJvdWdoJywgZnVuY3Rpb24gKCkgeworICAgIHdhaXRGb3JFdmVudCgnY2FucGxheScsIGZ1bmN0 aW9uICgpIHsKICAgICAgICAgaWYgKHdpbmRvdy5sYXlvdXRUZXN0Q29udHJvbGxlcikKICAgICAg ICAgICAgIHdpbmRvdy5sYXlvdXRUZXN0Q29udHJvbGxlci5ub3RpZnlEb25lKCk7CiAgICAgfSAp Owo=