156020 2016-03-30 03:59:02 -0700 [WinCairo][MediaFoundation] Video size is not always set. 2016-03-30 11:40:27 -0700 1 1 1 Unclassified WebKit WebCore Misc. WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 peavo webkit-unassigned achristensen bfulgham darin oldest_to_newest 1179041 0 peavo 2016-03-30 03:59:02 -0700 This happens because getting the video display control from the media session might fail the first time. If it fails the first time, we should try again later. 1179043 1 275192 peavo 2016-03-30 04:07:22 -0700 Created attachment 275192 Patch 1179065 2 275192 darin 2016-03-30 09:01:18 -0700 Comment on attachment 275192 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=275192&action=review > Source/WebCore/ChangeLog:9 > + Getting the video display control object from the media session might fail the first time. > + In case it fails, we should try again when setting the video size. Is keeping the video display control object an important optimization? Maybe we should just get it every time. > Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:353 > + COMPtr<IMFVideoDisplayControl> videoDisplay = getVideoDisplay(); Probably a little nicer to use auto here. > Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:778 > + if (FAILED(MFGetServicePtr()(m_mediaSession.get(), MR_VIDEO_RENDER_SERVICE, IID_PPV_ARGS(&m_videoDisplay)))) > + return nullptr; > + > + return m_videoDisplay; Checking FAILED explicitly here means we don’t trust that m_videoDisplay will be nullptr if the function calls. We check FAILED and explicitly return nullptr rather than just returning m_videoDisplay, which will be nullptr. But when this function is called again, whatever is stored in m_videoDisplay *will* be trusted. So we should omit the FAILED check since it does us no good. > Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:794 > + COMPtr<IMFVideoDisplayControl> videoDisplay = getVideoDisplay(); > + if (videoDisplay) { Would read nicer with auto and nesting the assignment in the if statement. if (auto videoDisplay = this->videoDisplay()) { > Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.h:138 > + COMPtr<IMFVideoDisplayControl> getVideoDisplay(); In WebKit coding style we do not use “get” in the names of functions like this one unless they produce values through "out arguments". We can either just call this videoDisplay or use some verb other than “get”. 1179122 3 peavo 2016-03-30 11:37:46 -0700 Thanks for reviewing! 1179125 4 peavo 2016-03-30 11:39:25 -0700 Committed r198848: <http://trac.webkit.org/changeset/198848> 275192 2016-03-30 04:07:22 -0700 2016-03-30 09:01:18 -0700 Patch bug-156020-20160330130746.patch text/plain 3971 peavo SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDE5ODgzMykKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE5IEBACisyMDE2LTAzLTMwICBQZXIgQXJu ZSBWb2xsYW4gIDxwZWF2b0BvdXRsb29rLmNvbT4KKworICAgICAgICBbV2luQ2Fpcm9dW01lZGlh Rm91bmRhdGlvbl0gVmlkZW8gc2l6ZSBpcyBub3QgYWx3YXlzIHNldC4KKyAgICAgICAgaHR0cHM6 Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTE1NjAyMAorCisgICAgICAgIFJldmll d2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIEdldHRpbmcgdGhlIHZpZGVvIGRpc3Bs YXkgY29udHJvbCBvYmplY3QgZnJvbSB0aGUgbWVkaWEgc2Vzc2lvbiBtaWdodCBmYWlsIHRoZSBm aXJzdCB0aW1lLgorICAgICAgICBJbiBjYXNlIGl0IGZhaWxzLCB3ZSBzaG91bGQgdHJ5IGFnYWlu IHdoZW4gc2V0dGluZyB0aGUgdmlkZW8gc2l6ZS4KKworICAgICAgICAqIHBsYXRmb3JtL2dyYXBo aWNzL3dpbi9NZWRpYVBsYXllclByaXZhdGVNZWRpYUZvdW5kYXRpb24uY3BwOgorICAgICAgICAo V2ViQ29yZTo6TWVkaWFQbGF5ZXJQcml2YXRlTWVkaWFGb3VuZGF0aW9uOjpzZXRTaXplKToKKyAg ICAgICAgKFdlYkNvcmU6Ok1lZGlhUGxheWVyUHJpdmF0ZU1lZGlhRm91bmRhdGlvbjo6Z2V0Vmlk ZW9EaXNwbGF5KToKKyAgICAgICAgKFdlYkNvcmU6Ok1lZGlhUGxheWVyUHJpdmF0ZU1lZGlhRm91 bmRhdGlvbjo6b25Ub3BvbG9neVNldCk6CisgICAgICAgICogcGxhdGZvcm0vZ3JhcGhpY3Mvd2lu L01lZGlhUGxheWVyUHJpdmF0ZU1lZGlhRm91bmRhdGlvbi5oOgorCiAyMDE2LTAzLTMwICBZb3Vl bm4gRmFibGV0ICA8eW91ZW5uLmZhYmxldEBjcmYuY2Fub24uZnI+CiAKICAgICAgICAgQmluZGlu ZyBnZW5lcmF0b3Igc2hvdWxkIGFsbG93IHBhc3NpbmcgRE9NIG9iamVjdHMgcGFyYW1ldGVycyBh cyByZWZlcmVuY2VzCkluZGV4OiBTb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9ncmFwaGljcy93aW4v TWVkaWFQbGF5ZXJQcml2YXRlTWVkaWFGb3VuZGF0aW9uLmNwcAo9PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3Vy Y2UvV2ViQ29yZS9wbGF0Zm9ybS9ncmFwaGljcy93aW4vTWVkaWFQbGF5ZXJQcml2YXRlTWVkaWFG b3VuZGF0aW9uLmNwcAkocmV2aXNpb24gMTk4ODMyKQorKysgU291cmNlL1dlYkNvcmUvcGxhdGZv cm0vZ3JhcGhpY3Mvd2luL01lZGlhUGxheWVyUHJpdmF0ZU1lZGlhRm91bmRhdGlvbi5jcHAJKHdv cmtpbmcgY29weSkKQEAgLTM1MCw3ICszNTAsOCBAQCB2b2lkIE1lZGlhUGxheWVyUHJpdmF0ZU1l ZGlhRm91bmRhdGlvbjo6CiB7CiAgICAgbV9zaXplID0gc2l6ZTsKIAotICAgIGlmICghbV92aWRl b0Rpc3BsYXkpCisgICAgQ09NUHRyPElNRlZpZGVvRGlzcGxheUNvbnRyb2w+IHZpZGVvRGlzcGxh eSA9IGdldFZpZGVvRGlzcGxheSgpOworICAgIGlmICghdmlkZW9EaXNwbGF5KQogICAgICAgICBy ZXR1cm47CiAKICAgICBJbnRQb2ludCBwb3NpdGlvbkluV2luZG93KG1fbGFzdFBhaW50UmVjdC5s b2NhdGlvbigpKTsKQEAgLTM3OSw3ICszODAsNyBAQCB2b2lkIE1lZGlhUGxheWVyUHJpdmF0ZU1l ZGlhRm91bmRhdGlvbjo6CiAgICAgICAgIDo6TW92ZVdpbmRvdyhtX2h3bmRWaWRlbywgeCwgeSwg dywgaCwgRkFMU0UpOwogCiAgICAgUkVDVCByYyA9IHsgMCwgMCwgdywgaCB9OwotICAgIG1fdmlk ZW9EaXNwbGF5LT5TZXRWaWRlb1Bvc2l0aW9uKG51bGxwdHIsICZyYyk7CisgICAgdmlkZW9EaXNw bGF5LT5TZXRWaWRlb1Bvc2l0aW9uKG51bGxwdHIsICZyYyk7CiB9CiAKIHZvaWQgTWVkaWFQbGF5 ZXJQcml2YXRlTWVkaWFGb3VuZGF0aW9uOjpwYWludChHcmFwaGljc0NvbnRleHQmIGNvbnRleHQs IGNvbnN0IEZsb2F0UmVjdCYgcmVjdCkKQEAgLTc2Myw2ICs3NjQsMjAgQEAgYm9vbCBNZWRpYVBs YXllclByaXZhdGVNZWRpYUZvdW5kYXRpb246OgogICAgIHJldHVybiB0cnVlOwogfQogCitDT01Q dHI8SU1GVmlkZW9EaXNwbGF5Q29udHJvbD4gTWVkaWFQbGF5ZXJQcml2YXRlTWVkaWFGb3VuZGF0 aW9uOjpnZXRWaWRlb0Rpc3BsYXkoKQoreworICAgIGlmIChtX3ZpZGVvRGlzcGxheSkKKyAgICAg ICAgcmV0dXJuIG1fdmlkZW9EaXNwbGF5OworCisgICAgaWYgKCFNRkdldFNlcnZpY2VQdHIoKSkK KyAgICAgICAgcmV0dXJuIG51bGxwdHI7CisKKyAgICBpZiAoRkFJTEVEKE1GR2V0U2VydmljZVB0 cigpKG1fbWVkaWFTZXNzaW9uLmdldCgpLCBNUl9WSURFT19SRU5ERVJfU0VSVklDRSwgSUlEX1BQ Vl9BUkdTKCZtX3ZpZGVvRGlzcGxheSkpKSkKKyAgICAgICAgcmV0dXJuIG51bGxwdHI7CisKKyAg ICByZXR1cm4gbV92aWRlb0Rpc3BsYXk7Cit9CisKIHZvaWQgTWVkaWFQbGF5ZXJQcml2YXRlTWVk aWFGb3VuZGF0aW9uOjpvbkNyZWF0ZWRNZWRpYVNvdXJjZSgpCiB7CiAgICAgaWYgKCFjcmVhdGVU b3BvbG9neUZyb21Tb3VyY2UoKSkKQEAgLTc3NSwxMyArNzkwLDEwIEBAIHZvaWQgTWVkaWFQbGF5 ZXJQcml2YXRlTWVkaWFGb3VuZGF0aW9uOjoKIAogdm9pZCBNZWRpYVBsYXllclByaXZhdGVNZWRp YUZvdW5kYXRpb246Om9uVG9wb2xvZ3lTZXQoKQogewotICAgIGlmICghTUZHZXRTZXJ2aWNlUHRy KCkpCi0gICAgICAgIHJldHVybjsKLQotICAgIGlmIChTVUNDRUVERUQoTUZHZXRTZXJ2aWNlUHRy KCkobV9tZWRpYVNlc3Npb24uZ2V0KCksIE1SX1ZJREVPX1JFTkRFUl9TRVJWSUNFLCBJSURfUFBW X0FSR1MoJm1fdmlkZW9EaXNwbGF5KSkpKSB7Ci0gICAgICAgIEFTU0VSVChtX3ZpZGVvRGlzcGxh eSk7CisgICAgQ09NUHRyPElNRlZpZGVvRGlzcGxheUNvbnRyb2w+IHZpZGVvRGlzcGxheSA9IGdl dFZpZGVvRGlzcGxheSgpOworICAgIGlmICh2aWRlb0Rpc3BsYXkpIHsKICAgICAgICAgUkVDVCBy YyA9IHsgMCwgMCwgbV9zaXplLndpZHRoKCksIG1fc2l6ZS5oZWlnaHQoKSB9OwotICAgICAgICBt X3ZpZGVvRGlzcGxheS0+U2V0VmlkZW9Qb3NpdGlvbihudWxscHRyLCAmcmMpOworICAgICAgICB2 aWRlb0Rpc3BsYXktPlNldFZpZGVvUG9zaXRpb24obnVsbHB0ciwgJnJjKTsKICAgICB9CiAKICAg ICBtX3JlYWR5U3RhdGUgPSBNZWRpYVBsYXllcjo6SGF2ZUZ1dHVyZURhdGE7CkluZGV4OiBTb3Vy Y2UvV2ViQ29yZS9wbGF0Zm9ybS9ncmFwaGljcy93aW4vTWVkaWFQbGF5ZXJQcml2YXRlTWVkaWFG b3VuZGF0aW9uLmgKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vZ3JhcGhp Y3Mvd2luL01lZGlhUGxheWVyUHJpdmF0ZU1lZGlhRm91bmRhdGlvbi5oCShyZXZpc2lvbiAxOTg4 MzIpCisrKyBTb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9ncmFwaGljcy93aW4vTWVkaWFQbGF5ZXJQ cml2YXRlTWVkaWFGb3VuZGF0aW9uLmgJKHdvcmtpbmcgY29weSkKQEAgLTEzNSw2ICsxMzUsOCBA QCBwcml2YXRlOgogICAgIGJvb2wgY3JlYXRlT3V0cHV0Tm9kZShDT01QdHI8SU1GU3RyZWFtRGVz Y3JpcHRvcj4gc291cmNlU0QsIENPTVB0cjxJTUZUb3BvbG9neU5vZGU+Jik7CiAgICAgYm9vbCBj cmVhdGVTb3VyY2VTdHJlYW1Ob2RlKENPTVB0cjxJTUZTdHJlYW1EZXNjcmlwdG9yPiBzb3VyY2VT RCwgQ09NUHRyPElNRlRvcG9sb2d5Tm9kZT4mKTsKIAorICAgIENPTVB0cjxJTUZWaWRlb0Rpc3Bs YXlDb250cm9sPiBnZXRWaWRlb0Rpc3BsYXkoKTsKKwogICAgIHZvaWQgb25DcmVhdGVkTWVkaWFT b3VyY2UoKTsKICAgICB2b2lkIG9uVG9wb2xvZ3lTZXQoKTsKIAo=