58312 2011-04-12 03:29:54 -0700 REGRESSION(r83561): doesn't pause in-window playback during fullscreen playback 2011-04-13 08:51:37 -0700 1 1 1 Unclassified WebKit Media 528+ (Nightly build) PC OS X 10.5 RESOLVED FIXED P2 Normal --- 1 pnormand webkit-unassigned mrobinson oldest_to_newest 383917 0 pnormand 2011-04-12 03:29:54 -0700 The videoValve is not linked if fpsdisplaysink is used. 383918 1 89183 pnormand 2011-04-12 03:32:02 -0700 Created attachment 89183 proposed patch 383987 2 89183 mrobinson 2011-04-12 07:26:24 -0700 Comment on attachment 89183 proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=89183&action=review Is it possible to include a test with this change? > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1714 > if (!m_fpsSink) { > gst_bin_add(GST_BIN(m_videoSinkBin), m_webkitVideoSink); > + actualVideoSink = m_webkitVideoSink; > + } > + I think it makes sense to turn if (!m_fpsSink) into an else statement here. 384067 3 pnormand 2011-04-12 09:33:03 -0700 (In reply to comment #2) > (From update of attachment 89183 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=89183&action=review > > Is it possible to include a test with this change? > > > I don't think this can be tested. The fact that we have 2 video-sinks is internal to the private media-player :/ Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1714 > > if (!m_fpsSink) { > > gst_bin_add(GST_BIN(m_videoSinkBin), m_webkitVideoSink); > > + actualVideoSink = m_webkitVideoSink; > > + } > > + > > I think it makes sense to turn if (!m_fpsSink) into an else statement here. OK. I can attach a new patch or do that tweak before landing if you r+ this. 384071 4 pnormand 2011-04-12 09:40:18 -0700 (In reply to comment #2) > (From update of attachment 89183 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=89183&action=review > > Is it possible to include a test with this change? > > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1714 > > if (!m_fpsSink) { > > gst_bin_add(GST_BIN(m_videoSinkBin), m_webkitVideoSink); > > + actualVideoSink = m_webkitVideoSink; > > + } > > + > > I think it makes sense to turn if (!m_fpsSink) into an else statement here. Hum sorry but I don't think so, finally :) m_fpsSink can be set to 0 in 2 cases just above this code, in the if (m_fpsSink) test. Turning this if (!m_fpsSink) to a else would require to duplicate the code, I think. 384915 5 mrobinson 2011-04-13 08:41:54 -0700 (In reply to comment #3) > (In reply to comment #2) > > (From update of attachment 89183 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=89183&action=review > > Is it possible to include a test with this change? > I don't think this can be tested. The fact that we have 2 video-sinks is internal to the private media-player :/ Alright. Please include a line in the ChangeLog explaining the lack of test, per typical WebCore ChangeLog style. :) 384919 6 pnormand 2011-04-13 08:51:37 -0700 Committed r83741: <http://trac.webkit.org/changeset/83741> 89183 2011-04-12 03:32:02 -0700 2011-04-13 08:42:39 -0700 proposed patch proposed-patch.patch text/plain 3582 pnormand RnJvbSAyYTRlNTA0ZGE2YjA1MDJiODM5ZDU4NmU2Yzg4OWNlODAxMzY0OTQ5IE1vbiBTZXAgMTcg MDA6MDA6MDAgMjAwMQpGcm9tOiBQaGlsaXBwZSBOb3JtYW5kIDxwbm9ybWFuZEBpZ2FsaWEuY29t PgpEYXRlOiBUdWUsIDEyIEFwciAyMDExIDEyOjMzOjA5ICswMjAwClN1YmplY3Q6IFtQQVRDSF0g cHJvcG9zZWQgcGF0Y2gKCi0tLQogU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nICAgICAgICAgICAg ICAgICAgICAgICAgICAgfCAgIDEzICsrKysrKysrKysrKysKIC4uLi9nc3RyZWFtZXIvTWVkaWFQ bGF5ZXJQcml2YXRlR1N0cmVhbWVyLmNwcCAgICAgIHwgICAxOCArKysrKysrKy0tLS0tLS0tLS0K IDIgZmlsZXMgY2hhbmdlZCwgMjEgaW5zZXJ0aW9ucygrKSwgMTAgZGVsZXRpb25zKC0pCgpkaWZm IC0tZ2l0IGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nIGIvU291cmNlL1dlYkNvcmUvQ2hhbmdl TG9nCmluZGV4IGI1ZjY2ZGUuLmVhMDdiNjQgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJDb3JlL0No YW5nZUxvZworKysgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKQEAgLTEsMyArMSwxNiBAQAor MjAxMS0wNC0xMiAgUGhpbGlwcGUgTm9ybWFuZCAgPHBub3JtYW5kQGlnYWxpYS5jb20+CisKKyAg ICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgUkVHUkVTU0lPTihy ODM1NjEpOiBkb2Vzbid0IHBhdXNlIGluLXdpbmRvdyBwbGF5YmFjayBkdXJpbmcgZnVsbHNjcmVl biBwbGF5YmFjaworICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/ aWQ9NTgzMTIKKworICAgICAgICBNYWtlIHN1cmUgdG8gbGluayB0aGUgdmlkZW9WYWx2ZSBpbiBh bGwgY2FzZXMsIGZwc2Rpc3BsYXlzaW5rCisgICAgICAgIGJlaW5nIHVzZWQgb3Igbm90LgorCisg ICAgICAgICogcGxhdGZvcm0vZ3JhcGhpY3MvZ3N0cmVhbWVyL01lZGlhUGxheWVyUHJpdmF0ZUdT dHJlYW1lci5jcHA6CisgICAgICAgIChXZWJDb3JlOjpNZWRpYVBsYXllclByaXZhdGVHU3RyZWFt ZXI6OmNyZWF0ZUdTVFBsYXlCaW4pOgorCiAyMDExLTA0LTA2ICBQaGlsaXBwZSBOb3JtYW5kICA8 cG5vcm1hbmRAaWdhbGlhLmNvbT4KIAogICAgICAgICBSZXZpZXdlZCBieSBNYXJ0aW4gUm9iaW5z b24uCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9ncmFwaGljcy9nc3RyZWFt ZXIvTWVkaWFQbGF5ZXJQcml2YXRlR1N0cmVhbWVyLmNwcCBiL1NvdXJjZS9XZWJDb3JlL3BsYXRm b3JtL2dyYXBoaWNzL2dzdHJlYW1lci9NZWRpYVBsYXllclByaXZhdGVHU3RyZWFtZXIuY3BwCmlu ZGV4IGVlM2Q0NzMuLjczMzZjODAgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3Jt L2dyYXBoaWNzL2dzdHJlYW1lci9NZWRpYVBsYXllclByaXZhdGVHU3RyZWFtZXIuY3BwCisrKyBi L1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL2dyYXBoaWNzL2dzdHJlYW1lci9NZWRpYVBsYXllclBy aXZhdGVHU3RyZWFtZXIuY3BwCkBAIC0xNjgyLDggKzE2ODIsOCBAQCB2b2lkIE1lZGlhUGxheWVy UHJpdmF0ZUdTdHJlYW1lcjo6Y3JlYXRlR1NUUGxheUJpbigpCiAgICAgZ3N0X29iamVjdF91bnJl ZihHU1RfT0JKRUNUKHNyY1BhZCkpOwogICAgIGdzdF9vYmplY3RfdW5yZWYoR1NUX09CSkVDVChz aW5rUGFkKSk7CiAKKyAgICBHc3RFbGVtZW50KiBhY3R1YWxWaWRlb1NpbmsgPSAwOwogICAgIG1f ZnBzU2luayA9IGdzdF9lbGVtZW50X2ZhY3RvcnlfbWFrZSgiZnBzZGlzcGxheXNpbmsiLCAic2lu ayIpOwotCiAgICAgaWYgKG1fZnBzU2luaykgewogICAgICAgICAvLyBUaGUgdmVyYm9zZSBwcm9w ZXJ0eSBoYXMgYmVlbiBhZGRlZCBpbiAtYmFkIDAuMTAuMjIuIE1ha2luZwogICAgICAgICAvLyB0 aGlzIHdob2xlIGNvZGUgZGVwZW5kIG9uIGl0IGJlY2F1c2Ugd2UgZG9uJ3Qgd2FudApAQCAtMTcw MCwxMiArMTcwMCw3IEBAIHZvaWQgTWVkaWFQbGF5ZXJQcml2YXRlR1N0cmVhbWVyOjpjcmVhdGVH U1RQbGF5QmluKCkKICAgICAgICAgICAgIGlmIChnX29iamVjdF9jbGFzc19maW5kX3Byb3BlcnR5 KEdfT0JKRUNUX0dFVF9DTEFTUyhtX2Zwc1NpbmspLCAidmlkZW8tc2luayIpKSB7CiAgICAgICAg ICAgICAgICAgZ19vYmplY3Rfc2V0KG1fZnBzU2luaywgInZpZGVvLXNpbmsiLCBtX3dlYmtpdFZp ZGVvU2luaywgTlVMTCk7CiAgICAgICAgICAgICAgICAgZ3N0X2Jpbl9hZGQoR1NUX0JJTihtX3Zp ZGVvU2lua0JpbiksIG1fZnBzU2luayk7Ci0jaWYgR1NUX0NIRUNLX1ZFUlNJT04oMCwgMTAsIDMw KQotICAgICAgICAgICAgICAgIC8vIEZhc3RlciBlbGVtZW50cyBsaW5raW5nLCBpZiBwb3NzaWJs ZS4KLSAgICAgICAgICAgICAgICBnc3RfZWxlbWVudF9saW5rX3BhZHNfZnVsbChxdWV1ZSwgInNy YyIsIG1fZnBzU2luaywgInNpbmsiLCBHU1RfUEFEX0xJTktfQ0hFQ0tfTk9USElORyk7Ci0jZWxz ZQotICAgICAgICAgICAgICAgIGdzdF9lbGVtZW50X2xpbmsocXVldWUsIG1fZnBzU2luayk7Ci0j ZW5kaWYKKyAgICAgICAgICAgICAgICBhY3R1YWxWaWRlb1NpbmsgPSBtX2Zwc1Npbms7CiAgICAg ICAgICAgICB9IGVsc2UKICAgICAgICAgICAgICAgICBtX2Zwc1NpbmsgPSAwOwogICAgICAgICB9 IGVsc2UKQEAgLTE3MTQsMTQgKzE3MDksMTcgQEAgdm9pZCBNZWRpYVBsYXllclByaXZhdGVHU3Ry ZWFtZXI6OmNyZWF0ZUdTVFBsYXlCaW4oKQogCiAgICAgaWYgKCFtX2Zwc1NpbmspIHsKICAgICAg ICAgZ3N0X2Jpbl9hZGQoR1NUX0JJTihtX3ZpZGVvU2lua0JpbiksIG1fd2Via2l0VmlkZW9TaW5r KTsKKyAgICAgICAgYWN0dWFsVmlkZW9TaW5rID0gbV93ZWJraXRWaWRlb1Npbms7CisgICAgfQor CisgICAgQVNTRVJUKGFjdHVhbFZpZGVvU2luayk7CiAjaWYgR1NUX0NIRUNLX1ZFUlNJT04oMCwg MTAsIDMwKQogICAgICAgICAvLyBGYXN0ZXIgZWxlbWVudHMgbGlua2luZywgaWYgcG9zc2libGUu CiAgICAgICAgIGdzdF9lbGVtZW50X2xpbmtfcGFkc19mdWxsKHF1ZXVlLCAic3JjIiwgaWRlbnRp dHksICJzaW5rIiwgR1NUX1BBRF9MSU5LX0NIRUNLX05PVEhJTkcpOwotICAgICAgICBnc3RfZWxl bWVudF9saW5rX3BhZHNfZnVsbChpZGVudGl0eSwgInNyYyIsIG1fd2Via2l0VmlkZW9TaW5rLCAi c2luayIsIEdTVF9QQURfTElOS19DSEVDS19OT1RISU5HKTsKKyAgICAgICAgZ3N0X2VsZW1lbnRf bGlua19wYWRzX2Z1bGwoaWRlbnRpdHksICJzcmMiLCBhY3R1YWxWaWRlb1NpbmssICJzaW5rIiwg R1NUX1BBRF9MSU5LX0NIRUNLX05PVEhJTkcpOwogI2Vsc2UKLSAgICAgICAgZ3N0X2VsZW1lbnRf bGlua19tYW55KHF1ZXVlLCBpZGVudGl0eSwgbV93ZWJraXRWaWRlb1NpbmssIE5VTEwpOworICAg ICAgICBnc3RfZWxlbWVudF9saW5rX21hbnkocXVldWUsIGlkZW50aXR5LCBhY3R1YWxWaWRlb1Np bmssIE5VTEwpOwogI2VuZGlmCi0gICAgfQogCiAgICAgLy8gQWRkIGEgZ2hvc3RwYWQgdG8gdGhl IGJpbiBzbyBpdCBjYW4gcHJveHkgdG8gdGVlLgogICAgIEdzdFBhZCogcGFkID0gZ3N0X2VsZW1l bnRfZ2V0X3N0YXRpY19wYWQodmlkZW9UZWUsICJzaW5rIik7Ci0tIAoxLjcuNC4x