61160 2011-05-19 17:57:03 -0700 GTK: media/video-src-invalid-poster.html Failed 2011-07-07 04:18:55 -0700 1 1 1 Unclassified WebKit Media 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED http://build.webkit.org/results/GTK%20Linux%2032-bit%20Release/r86904%20(13918)/results.html P2 Normal --- 1 jer.noble webkit-unassigned eric.carlson pnormand oldest_to_newest 406787 0 jer.noble 2011-05-19 17:57:03 -0700 On GTK, the media/video-src-invalid-ponter.html test gives the correct width but the wrong height. 406790 1 jer.noble 2011-05-19 18:03:43 -0700 Committed r86911 adding the new test to the GTK skipped list. 410453 2 94976 pnormand 2011-05-26 08:18:53 -0700 Created attachment 94976 proposed patch 410454 3 pnormand 2011-05-26 08:20:17 -0700 (In reply to comment #0) > On GTK, the media/video-src-invalid-ponter.html test gives the correct width but the wrong height. Are you sure? What I saw locally is that sometimes the test times out, otherwise it succeeds locally. Hence the attached patch to avoid the timeout 410526 4 jer.noble 2011-05-26 09:39:43 -0700 (In reply to comment #3) > (In reply to comment #0) > > On GTK, the media/video-src-invalid-ponter.html test gives the correct width but the wrong height. > > Are you sure? What I saw locally is that sometimes the test times out, otherwise it succeeds locally. Definitely. Check the test results from the URL field above. It shows the video sized to 73 (correct) by 150 (incorrect, but probably the default video element width). However, if it's not reproducing any longer, then aces. ;) > Hence the attached patch to avoid the timeout So the patch seems to significantly change the behavior of the test by calling |video.load()|. Shouldn't we see the correct behavior without calling load()? The original test gave the video element a whole second to resize and display the poster. Is that not enough time? 410529 5 pnormand 2011-05-26 09:45:42 -0700 (In reply to comment #4) > (In reply to comment #3) > > (In reply to comment #0) > > > On GTK, the media/video-src-invalid-ponter.html test gives the correct width but the wrong height. > > > > Are you sure? What I saw locally is that sometimes the test times out, otherwise it succeeds locally. > > Definitely. Check the test results from the URL field above. It shows the video sized to 73 (correct) by 150 (incorrect, but probably the default video element width). > Oh I didn't that link indeed. Yes it's odd. I recall the Release bot might need a GStreamer upgrade to be in par with the Debug bots. At least locally the test passes but I use a fairly recent GStreamer version. > However, if it's not reproducing any longer, then aces. ;) > > > Hence the attached patch to avoid the timeout > > So the patch seems to significantly change the behavior of the test by calling |video.load()|. Shouldn't we see the correct behavior without calling load()? The original test gave the video element a whole second to resize and display the poster. Is that not enough time? On my slow laptop I could see the timeout happen. The GTK bots are know to not be lighning fast either... In general for media tests (at least) I believe we should react more on events instead of waiting on something to happen before a XXXX ms timeout to fire. 410539 6 jer.noble 2011-05-26 09:56:03 -0700 (In reply to comment #5) > (In reply to comment #4) > > So the patch seems to significantly change the behavior of the test by calling |video.load()|. Shouldn't we see the correct behavior without calling load()? The original test gave the video element a whole second to resize and display the poster. Is that not enough time? > > On my slow laptop I could see the timeout happen. The GTK bots are know to not be lighning fast either... > In general for media tests (at least) I believe we should react more on events instead of waiting on something to happen before a XXXX ms timeout to fire. Okay, I'd be fine with getting rid of the 1s timeout. What about |video.load()|? Does the test pass on GTK+ without that, but also without the timeout? 414698 7 pnormand 2011-06-03 00:45:00 -0700 (In reply to comment #6) > (In reply to comment #5) > > (In reply to comment #4) > > > So the patch seems to significantly change the behavior of the test by calling |video.load()|. Shouldn't we see the correct behavior without calling load()? The original test gave the video element a whole second to resize and display the poster. Is that not enough time? > > > > On my slow laptop I could see the timeout happen. The GTK bots are know to not be lighning fast either... > > In general for media tests (at least) I believe we should react more on events instead of waiting on something to happen before a XXXX ms timeout to fire. > > Okay, I'd be fine with getting rid of the 1s timeout. What about |video.load()|? Does the test pass on GTK+ without that, but also without the timeout? Doesn't pass because if load() isn't called there's no loadstart event. 432763 8 pnormand 2011-07-06 07:27:50 -0700 Can this be reviewed please? Eric, would you have some time to look at it? Thanks! 432788 9 94976 eric.carlson 2011-07-06 08:17:42 -0700 Comment on attachment 94976 proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=94976&action=review > LayoutTests/ChangeLog:9 > + Wait loadstart event to check the poster dimensions and avoid a > + potential timeout of the test. Nit: Wait for loadstart event... 433386 10 pnormand 2011-07-07 04:18:55 -0700 Committed r90554: <http://trac.webkit.org/changeset/90554> 94976 2011-05-26 08:18:53 -0700 2011-07-06 08:17:42 -0700 proposed patch proposed-patch.patch text/plain 2825 pnormand RnJvbSBjMzU1MjJhNmJjYjNjYzM3NTIwYTNmMzBjMzU5ODU2MzRlMDZjODYwIE1vbiBTZXAgMTcg MDA6MDA6MDAgMjAwMQpGcm9tOiBQaGlsaXBwZSBOb3JtYW5kIDxwbm9ybWFuZEBpZ2FsaWEuY29t PgpEYXRlOiBUaHUsIDI2IE1heSAyMDExIDE3OjE4OjE4ICswMjAwClN1YmplY3Q6IFtQQVRDSF0g cHJvcG9zZWQgcGF0Y2gKCi0tLQogTGF5b3V0VGVzdHMvQ2hhbmdlTG9nICAgICAgICAgICAgICAg ICAgICAgICAgICAgICAgfCAgIDE0ICsrKysrKysrKysrKysrCiAuLi4vbWVkaWEvdmlkZW8tc3Jj LWludmFsaWQtcG9zdGVyLWV4cGVjdGVkLnR4dCAgICB8ICAgIDIgKysKIExheW91dFRlc3RzL21l ZGlhL3ZpZGVvLXNyYy1pbnZhbGlkLXBvc3Rlci5odG1sICAgIHwgICAgNiArKysrLS0KIExheW91 dFRlc3RzL3BsYXRmb3JtL2d0ay9Ta2lwcGVkICAgICAgICAgICAgICAgICAgIHwgICAgMyAtLS0K IDQgZmlsZXMgY2hhbmdlZCwgMjAgaW5zZXJ0aW9ucygrKSwgNSBkZWxldGlvbnMoLSkKCmRpZmYg LS1naXQgYS9MYXlvdXRUZXN0cy9DaGFuZ2VMb2cgYi9MYXlvdXRUZXN0cy9DaGFuZ2VMb2cKaW5k ZXggOWMyNWIwYy4uYzYzZTU4NiAxMDA2NDQKLS0tIGEvTGF5b3V0VGVzdHMvQ2hhbmdlTG9nCisr KyBiL0xheW91dFRlc3RzL0NoYW5nZUxvZwpAQCAtMSw1ICsxLDE5IEBACiAyMDExLTA1LTI2ICBQ aGlsaXBwZSBOb3JtYW5kICA8cG5vcm1hbmRAaWdhbGlhLmNvbT4KIAorICAgICAgICBSZXZpZXdl ZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBHVEs6IG1lZGlhL3ZpZGVvLXNyYy1pbnZh bGlkLXBvc3Rlci5odG1sIEZhaWxlZAorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9z aG93X2J1Zy5jZ2k/aWQ9NjExNjAKKworICAgICAgICBXYWl0IGxvYWRzdGFydCBldmVudCB0byBj aGVjayB0aGUgcG9zdGVyIGRpbWVuc2lvbnMgYW5kIGF2b2lkIGEKKyAgICAgICAgcG90ZW50aWFs IHRpbWVvdXQgb2YgdGhlIHRlc3QuCisKKyAgICAgICAgKiBtZWRpYS92aWRlby1zcmMtaW52YWxp ZC1wb3N0ZXItZXhwZWN0ZWQudHh0OgorICAgICAgICAqIG1lZGlhL3ZpZGVvLXNyYy1pbnZhbGlk LXBvc3Rlci5odG1sOgorICAgICAgICAqIHBsYXRmb3JtL2d0ay9Ta2lwcGVkOgorCisyMDExLTA1 LTI2ICBQaGlsaXBwZSBOb3JtYW5kICA8cG5vcm1hbmRAaWdhbGlhLmNvbT4KKwogICAgICAgICBV bnJldmlld2VkLCBza2lwIGZsYWt5IHRlc3Qgb24gR1RLLgogCiAgICAgICAgIFtHVEtdIGh0dHAv dGVzdHMvZXZlbnRzb3VyY2Uvd29ya2Vycy9ldmVudHNvdXJjZS1zaW1wbGUuaHRtbCBpcyBmbGFr eSBjcmFzaApkaWZmIC0tZ2l0IGEvTGF5b3V0VGVzdHMvbWVkaWEvdmlkZW8tc3JjLWludmFsaWQt cG9zdGVyLWV4cGVjdGVkLnR4dCBiL0xheW91dFRlc3RzL21lZGlhL3ZpZGVvLXNyYy1pbnZhbGlk LXBvc3Rlci1leHBlY3RlZC50eHQKaW5kZXggYmFiMDJhMC4uN2VlZDM4NCAxMDA2NDQKLS0tIGEv TGF5b3V0VGVzdHMvbWVkaWEvdmlkZW8tc3JjLWludmFsaWQtcG9zdGVyLWV4cGVjdGVkLnR4dAor KysgYi9MYXlvdXRUZXN0cy9tZWRpYS92aWRlby1zcmMtaW52YWxpZC1wb3N0ZXItZXhwZWN0ZWQu dHh0CkBAIC0xLDMgKzEsNSBAQAorUlVOKHZpZGVvLmxvYWQoKSkKK0VWRU5UKGxvYWRzdGFydCkK IEVYUEVDVEVEICh2aWRlby5jbGllbnRXaWR0aCA9PSAnNzYnKSBPSwogRVhQRUNURUQgKHZpZGVv LmNsaWVudEhlaWdodCA9PSAnMTAzJykgT0sKIEVORCBPRiBURVNUCmRpZmYgLS1naXQgYS9MYXlv dXRUZXN0cy9tZWRpYS92aWRlby1zcmMtaW52YWxpZC1wb3N0ZXIuaHRtbCBiL0xheW91dFRlc3Rz L21lZGlhL3ZpZGVvLXNyYy1pbnZhbGlkLXBvc3Rlci5odG1sCmluZGV4IDM1OGNmNjAuLjFiNmRl Y2IgMTAwNjQ0Ci0tLSBhL0xheW91dFRlc3RzL21lZGlhL3ZpZGVvLXNyYy1pbnZhbGlkLXBvc3Rl ci5odG1sCisrKyBiL0xheW91dFRlc3RzL21lZGlhL3ZpZGVvLXNyYy1pbnZhbGlkLXBvc3Rlci5o dG1sCkBAIC0yMSw2ICsyMSw4IEBACiAgICAgICAgIGVuZFRlc3QoKTsKICAgICB9CiAKLSAgICBs aXN0ZW5Gb3JXaWR0aEFuZEhlaWdodCg3NiwgMTAzLCBleHBlY3RlZCk7Ci0gICAgZmFpbFRlc3RJ bigxMDAwKTsKKyAgICBydW4oInZpZGVvLmxvYWQoKSIpOworICAgIHdhaXRGb3JFdmVudCgibG9h ZHN0YXJ0IiwgZnVuY3Rpb24gKCkgeworICAgICAgICBsaXN0ZW5Gb3JXaWR0aEFuZEhlaWdodCg3 NiwgMTAzLCBleHBlY3RlZCk7CisgICAgfSk7CiA8L3NjcmlwdD4KZGlmZiAtLWdpdCBhL0xheW91 dFRlc3RzL3BsYXRmb3JtL2d0ay9Ta2lwcGVkIGIvTGF5b3V0VGVzdHMvcGxhdGZvcm0vZ3RrL1Nr aXBwZWQKaW5kZXggYmQ1NjMxYi4uZTU1N2RiZiAxMDA2NDQKLS0tIGEvTGF5b3V0VGVzdHMvcGxh dGZvcm0vZ3RrL1NraXBwZWQKKysrIGIvTGF5b3V0VGVzdHMvcGxhdGZvcm0vZ3RrL1NraXBwZWQK QEAgLTE0NDgsOSArMTQ0OCw2IEBAIGh0dHAvdGVzdHMvYXBwY2FjaGUvaW50ZXJydXB0ZWQtdXBk YXRlLmh0bWwKICMgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTYxMTIy CiBodHRwL3Rlc3RzL25hdmlnYXRpb24vcG9zdC0zMDctcmVzcG9uc2UuaHRtbAogCi0jIGh0dHBz Oi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD02MTE2MAotbWVkaWEvdmlkZW8tc3Jj LWludmFsaWQtcG9zdGVyLmh0bWwKLQogIyBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1 Zy5jZ2k/aWQ9NjEzOTYKIGZhc3QvZXZlbnRzL3Njcm9sbC1pbi1zY2FsZWQtcGFnZS13aXRoLW92 ZXJmbG93LWhpZGRlbi5odG1sCiAKLS0gCjEuNy41LjE=