177016 2017-09-15 11:21:02 -0700 MediaPlayerPrivateMediaStreamAVFObjC::requestNotificationWhenReadyForVideoData should enqueue data if still useful 2017-09-15 12:55:27 -0700 1 1 1 Unclassified WebKit WebCore Misc. WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 youennf youennf commit-queue eric.carlson ggaren jer.noble webkit-bug-importer oldest_to_newest 1349407 0 youennf 2017-09-15 11:21:02 -0700 If it is dead, no need to do it. 1349408 1 youennf 2017-09-15 11:21:14 -0700 rdar://problem/33803695 1349409 2 320933 youennf 2017-09-15 11:22:49 -0700 Created attachment 320933 Patch 1349414 3 320933 jer.noble 2017-09-15 11:32:50 -0700 Comment on attachment 320933 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=320933&action=review > Source/WebCore/ChangeLog:11 > + (WebCore::MediaPlayerPrivateMediaStreamAVFObjC::requestNotificationWhenReadyForVideoData): exciting early in block to prevent enqueueing. Nit: "exiting". > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:401 > + auto weakThis = createWeakPtr(); > [m_sampleBufferDisplayLayer requestMediaDataWhenReadyOnQueue:dispatch_get_main_queue() usingBlock:^ { > + if (!weakThis) > + return; > + Note that this won't necessarily be correct on iOS WK1, as the "dispatch_get_main_queue()" won't necessarily be the WebThread, in which case, you'll assert in the weakThis getter. This isn't a problem in the MSE code, since that's not enabled on iOS, but it may be a problem here, since media streams are. 1349435 4 youennf 2017-09-15 12:01:56 -0700 > Note that this won't necessarily be correct on iOS WK1, as the > "dispatch_get_main_queue()" won't necessarily be the WebThread, in which > case, you'll assert in the weakThis getter. This isn't a problem in the MSE > code, since that's not enabled on iOS, but it may be a problem here, since > media streams are. I am not familiar with WebThread. I guess that if we are not in the WebThread, we could hop to it, through callOnMainThread probably?. Or is there a way to dispatch it to the WebThread directly? 1349436 5 jer.noble 2017-09-15 12:18:01 -0700 (In reply to youenn fablet from comment #4) > > Note that this won't necessarily be correct on iOS WK1, as the > > "dispatch_get_main_queue()" won't necessarily be the WebThread, in which > > case, you'll assert in the weakThis getter. This isn't a problem in the MSE > > code, since that's not enabled on iOS, but it may be a problem here, since > > media streams are. > > I am not familiar with WebThread. The basic gist is that, to get smooth scrolling on iOS in WK1, all the WebCore "main thread" functions run off the process's main thread, an on a named thread called "WebThread". > I guess that if we are not in the WebThread, we could hop to it, through > callOnMainThread probably?. Yes. > Or is there a way to dispatch it to the WebThread directly? No, there's not (but that would be great!). 1349441 6 320933 eric.carlson 2017-09-15 12:22:36 -0700 Comment on attachment 320933 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=320933&action=review >> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:401 >> + > > Note that this won't necessarily be correct on iOS WK1, as the "dispatch_get_main_queue()" won't necessarily be the WebThread, in which case, you'll assert in the weakThis getter. This isn't a problem in the MSE code, since that's not enabled on iOS, but it may be a problem here, since media streams are. We don't support media streams in WK1 so this should be OK. 1349442 7 jer.noble 2017-09-15 12:23:56 -0700 (In reply to Eric Carlson from comment #6) > Comment on attachment 320933 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=320933&action=review > > >> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:401 > >> + > > > > Note that this won't necessarily be correct on iOS WK1, as the "dispatch_get_main_queue()" won't necessarily be the WebThread, in which case, you'll assert in the weakThis getter. This isn't a problem in the MSE code, since that's not enabled on iOS, but it may be a problem here, since media streams are. > > We don't support media streams in WK1 so this should be OK. Oh, ok, never mind then. :) 1349446 8 320933 ap 2017-09-15 12:27:54 -0700 Comment on attachment 320933 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=320933&action=review > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:400 > + if (!weakThis) > + return; What if the object gets destroyed after this check, but before the rest of the code in the block runs? 1349451 9 youennf 2017-09-15 12:32:27 -0700 I think there is a plan to try removing the WebThread. 1349452 10 320933 youennf 2017-09-15 12:36:07 -0700 Comment on attachment 320933 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=320933&action=review >> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:400 >> + return; > > What if the object gets destroyed after this check, but before the rest of the code in the block runs? As I read it, the object destruction and this code block should both happen on the same thread. 1349458 11 320933 ggaren 2017-09-15 12:46:19 -0700 Comment on attachment 320933 Patch Have we converged to an r+ then? 1349460 12 jer.noble 2017-09-15 12:48:16 -0700 (In reply to Geoffrey Garen from comment #11) > Comment on attachment 320933 [details] > Patch > > Have we converged to an r+ then? Yep. 1349472 13 320933 commit-queue 2017-09-15 12:55:26 -0700 Comment on attachment 320933 Patch Clearing flags on attachment: 320933 Committed r222105: <http://trac.webkit.org/changeset/222105> 1349473 14 commit-queue 2017-09-15 12:55:27 -0700 All reviewed patches have been landed. Closing bug. 320933 2017-09-15 11:22:49 -0700 2017-09-15 12:55:26 -0700 Patch bug-177016-20170915112249.patch text/plain 1981 youennf U3VidmVyc2lvbiBSZXZpc2lvbjogMjIyMDY1CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggYmFiNjQxMzRmMTI2Yzkx NjkxYmI1ZmEyN2YyNTNiOWVmZjI2YTBjMi4uNzE3OTc2YTAwMDI3MGMzMjNiYWZlZTNkZTBkOWE3 MDgwNzgyNmE4NCAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSw1ICsxLDE3IEBACiAyMDE3LTA5LTE1ICBZb3Vl bm4gRmFibGV0ICA8eW91ZW5uQGFwcGxlLmNvbT4KIAorICAgICAgICBNZWRpYVBsYXllclByaXZh dGVNZWRpYVN0cmVhbUFWRk9iakM6OnJlcXVlc3ROb3RpZmljYXRpb25XaGVuUmVhZHlGb3JWaWRl b0RhdGEgc2hvdWxkIGVucXVldWUgZGF0YSBpZiBzdGlsbCB1c2VmdWwKKyAgICAgICAgaHR0cHM6 Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTE3NzAxNgorCisgICAgICAgIFJldmll d2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIE5vIGNoYW5nZSBvZiBiZWhhdmlvci4K KworICAgICAgICAqIHBsYXRmb3JtL2dyYXBoaWNzL2F2Zm91bmRhdGlvbi9vYmpjL01lZGlhUGxh eWVyUHJpdmF0ZU1lZGlhU3RyZWFtQVZGT2JqQy5tbToKKyAgICAgICAgKFdlYkNvcmU6Ok1lZGlh UGxheWVyUHJpdmF0ZU1lZGlhU3RyZWFtQVZGT2JqQzo6cmVxdWVzdE5vdGlmaWNhdGlvbldoZW5S ZWFkeUZvclZpZGVvRGF0YSk6IGV4Y2l0aW5nIGVhcmx5IGluIGJsb2NrIHRvIHByZXZlbnQgZW5x dWV1ZWluZy4KKworMjAxNy0wOS0xNSAgWW91ZW5uIEZhYmxldCAgPHlvdWVubkBhcHBsZS5jb20+ CisKICAgICAgICAgVXNlIHZlY3RvciBtYXAgcm91dGluZSBpbiBXZWJDb3JlIENhY2hlU3RvcmFn ZSBpbXBsZW1lbnRhdGlvbgogICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1 Zy5jZ2k/aWQ9MTc3MDAyCiAKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL2dy YXBoaWNzL2F2Zm91bmRhdGlvbi9vYmpjL01lZGlhUGxheWVyUHJpdmF0ZU1lZGlhU3RyZWFtQVZG T2JqQy5tbSBiL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL2dyYXBoaWNzL2F2Zm91bmRhdGlvbi9v YmpjL01lZGlhUGxheWVyUHJpdmF0ZU1lZGlhU3RyZWFtQVZGT2JqQy5tbQppbmRleCA1MmQ3NGQ2 ZWI1MTQxYWVjNGQzMzY0NDc5ODkwMGQ1MGVhOWRhNjY0Li5jNDJlNDYyNmQ2NTM2ZjU5MzY2MTI3 MmI4ZjhkMjRjMjY0ZjE1MjlhIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9n cmFwaGljcy9hdmZvdW5kYXRpb24vb2JqYy9NZWRpYVBsYXllclByaXZhdGVNZWRpYVN0cmVhbUFW Rk9iakMubW0KKysrIGIvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3MvYXZmb3VuZGF0 aW9uL29iamMvTWVkaWFQbGF5ZXJQcml2YXRlTWVkaWFTdHJlYW1BVkZPYmpDLm1tCkBAIC0zOTQs NyArMzk0LDExIEBAIHZvaWQgTWVkaWFQbGF5ZXJQcml2YXRlTWVkaWFTdHJlYW1BVkZPYmpDOjpl bnF1ZXVlVmlkZW9TYW1wbGUoTWVkaWFTdHJlYW1UcmFja1ByCiAKIHZvaWQgTWVkaWFQbGF5ZXJQ cml2YXRlTWVkaWFTdHJlYW1BVkZPYmpDOjpyZXF1ZXN0Tm90aWZpY2F0aW9uV2hlblJlYWR5Rm9y VmlkZW9EYXRhKCkKIHsKKyAgICBhdXRvIHdlYWtUaGlzID0gY3JlYXRlV2Vha1B0cigpOwogICAg IFttX3NhbXBsZUJ1ZmZlckRpc3BsYXlMYXllciByZXF1ZXN0TWVkaWFEYXRhV2hlblJlYWR5T25R dWV1ZTpkaXNwYXRjaF9nZXRfbWFpbl9xdWV1ZSgpIHVzaW5nQmxvY2s6XiB7CisgICAgICAgIGlm ICghd2Vha1RoaXMpCisgICAgICAgICAgICByZXR1cm47CisKICAgICAgICAgW21fc2FtcGxlQnVm ZmVyRGlzcGxheUxheWVyIHN0b3BSZXF1ZXN0aW5nTWVkaWFEYXRhXTsKIAogICAgICAgICB3aGls ZSAoIW1fcGVuZGluZ1ZpZGVvU2FtcGxlUXVldWUuaXNFbXB0eSgpKSB7Cg==