99592 2012-10-17 06:42:15 -0700 Add WebGraphicsContext3D::WebGraphicsUpdateVSyncTimeCallbackCHROMIUM 2012-10-24 09:05:06 -0700 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) All All RESOLVED WONTFIX P2 Normal --- 0 ajuma webkit-unassigned backer nduca oldest_to_newest 744209 0 169172 ajuma 2012-10-17 06:42:15 -0700 Created attachment 169172 Patch This is needed for Chromium issue 143466 (http://crbug.com/143466). 744432 1 169172 brianderson 2012-10-17 11:14:57 -0700 Comment on attachment 169172 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=169172&action=review > Source/Platform/chromium/public/WebGraphicsContext3D.h:147 > + virtual void onUpdateVSyncTime(int64_t) = 0; Can you use a more descriptive variable name so we know what the units are? Do you need two arguments (one for phase and one for frequency)? Also, other parts of the code have started moving towards base::TimeTicks and base::TimeDeltas. I'm not sure those are allowed in WebGraphicsContext3D.h though. If they are, we should use those instead. 744435 2 169172 nduca 2012-10-17 11:17:24 -0700 Comment on attachment 169172 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=169172&action=review > Source/Platform/chromium/public/WebGraphicsContext3D.h:145 > + class WebGraphicsUpdateVSyncTimeCallbackCHROMIUM { why do you need this? Can't you use the output surface system? 744605 3 ajuma 2012-10-17 14:14:39 -0700 (In reply to comment #2) > (From update of attachment 169172 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=169172&action=review > > > Source/Platform/chromium/public/WebGraphicsContext3D.h:145 > > + class WebGraphicsUpdateVSyncTimeCallbackCHROMIUM { > > why do you need this? Can't you use the output surface system? One big advantage of the new approach is that it works without modification for sending vsync updates to renderers on desktop Linux (whenever OML_GLX_sync_control is available). The downside of adapting this approach to go through the existing path is that the result will be Aura-specific. To avoid having two different paths for updating vsync state, we could move the existing code to the new path. This would mean that the GPU process would always be responsible for pushing out vsync updates to all compositors; in situations (as we currently have) where the browser process is the original source of this information, the browser would send the information to the GPU process and the GPU process would pass it on to renderers. Thoughts on this? (In reply to comment #1) > (From update of attachment 169172 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=169172&action=review > > > Source/Platform/chromium/public/WebGraphicsContext3D.h:147 > > + virtual void onUpdateVSyncTime(int64_t) = 0; > > Do you need two arguments (one for phase and one for frequency)? Yes, there should also be an argument for frequency (but note that OML_GLX_sync_control is currently not providing frequency information on Chrome OS devices). 744607 4 nduca 2012-10-17 14:17:35 -0700 Linux platforms are the only platforms where vsync originates in the gpu process. Everywhere else, its available in the browser process. So, I see the linuxes plumbing as a special case. We dont have a wgc3d in software mode, but we do have vsync. So, the use of outputsurface has to stay. 744609 5 nduca 2012-10-17 14:18:46 -0700 Also, the gpu process pushing vsync to compositors adds additional hops in the propagation of the signal on platforms where the signal is directly available. These hops have queuing delays, which while short in the minimum case, are very noisy and can often have 5ms bursts in them. No good. 749723 6 ajuma 2012-10-24 09:05:06 -0700 This is no longer needed for Chromium issue 143466 since we're going to use the existing output surface system instead. 169172 2012-10-17 06:42:15 -0700 2012-10-17 11:17:24 -0700 Patch WebKitPatchForIssue143466 text/plain 1483 ajuma ZGlmZiAtLWdpdCBhL1NvdXJjZS9QbGF0Zm9ybS9jaHJvbWl1bS9wdWJsaWMvV2ViR3JhcGhpY3ND b250ZXh0M0QuaCBiL1NvdXJjZS9QbGF0Zm9ybS9jaHJvbWl1bS9wdWJsaWMvV2ViR3JhcGhpY3ND b250ZXh0M0QuaAppbmRleCA4Zjg1ODUxLi41MWUyNDgxIDEwMDY0NAotLS0gYS9Tb3VyY2UvUGxh dGZvcm0vY2hyb21pdW0vcHVibGljL1dlYkdyYXBoaWNzQ29udGV4dDNELmgKKysrIGIvU291cmNl L1BsYXRmb3JtL2Nocm9taXVtL3B1YmxpYy9XZWJHcmFwaGljc0NvbnRleHQzRC5oCkBAIC0xNDIs NiArMTQyLDE0IEBAIHB1YmxpYzoKICAgICAgICAgdmlydHVhbCB+V2ViR3JhcGhpY3NNZW1vcnlB bGxvY2F0aW9uQ2hhbmdlZENhbGxiYWNrQ0hST01JVU0oKSB7IH0KICAgICB9OwogCisgICAgY2xh c3MgV2ViR3JhcGhpY3NVcGRhdGVWU3luY1RpbWVDYWxsYmFja0NIUk9NSVVNIHsKKyAgICBwdWJs aWM6CisgICAgICAgIHZpcnR1YWwgdm9pZCBvblVwZGF0ZVZTeW5jVGltZShpbnQ2NF90KSA9IDA7 CisKKyAgICBwcm90ZWN0ZWQ6CisgICAgICAgIHZpcnR1YWwgfldlYkdyYXBoaWNzVXBkYXRlVlN5 bmNUaW1lQ2FsbGJhY2tDSFJPTUlVTSgpIHsgfQorICAgIH07CisKICAgICAvLyBUaGlzIGRlc3Ry dWN0b3IgbmVlZHMgdG8gYmUgcHVibGljIHNvIHRoYXQgdXNpbmcgY2xhc3NlcyBjYW4gZGVzdHJv eSBpbnN0YW5jZXMgaWYgaW5pdGlhbGl6YXRpb24gZmFpbHMuCiAgICAgdmlydHVhbCB+V2ViR3Jh cGhpY3NDb250ZXh0M0QoKSB7IH0KIApAQCAtMTYzLDYgKzE3MSw5IEBAIHB1YmxpYzoKICAgICAv LyBHTF9DSFJPTUlVTV9ncHVfbWVtb3J5X21hbmFnZXIgLSBzZXRzIGNhbGxiYWNrIHRvIG9ic2Vy dmUgY2hhbmdlcyB0byBtZW1vcnkgYWxsb2NhdGlvbiBsaW1pdHMuCiAgICAgdmlydHVhbCB2b2lk IHNldE1lbW9yeUFsbG9jYXRpb25DaGFuZ2VkQ2FsbGJhY2tDSFJPTUlVTShXZWJHcmFwaGljc01l bW9yeUFsbG9jYXRpb25DaGFuZ2VkQ2FsbGJhY2tDSFJPTUlVTSogY2FsbGJhY2spIHsgfQogCisg ICAgLy8gU2V0cyBjYWxsYmFjayB0byByZWNlaXZlIHVwZGF0ZXMgdG8gc2NyZWVuIHJlZnJlc2gg dGltZS4KKyAgICB2aXJ0dWFsIHZvaWQgc2V0VXBkYXRlVlN5bmNUaW1lQ2FsbGJhY2tDSFJPTUlV TShXZWJHcmFwaGljc1VwZGF0ZVZTeW5jVGltZUNhbGxiYWNrQ0hST01JVU0qIGNhbGxiYWNrKSB7 IH0KKwogICAgIC8vIEdMX0VYVF9kaXNjYXJkX2ZyYW1lYnVmZmVyIC0gZGlzY2FyZC9lbnN1cmUg ZXhpc3RhbmNlIG9mIHN1cmZhY2UgYmFja2J1ZmZlci4KICAgICAvLyBGSVhNRTogbWFrZSB0aGVz ZSBwdXJlIHZpcnR1YWwgb25jZSB0aGV5IGFyZSBpbXBsZW1lbnRlZCBieSBjbGllbnRzLgogICAg IHZpcnR1YWwgdm9pZCBkaXNjYXJkRnJhbWVidWZmZXJFWFQoV0dDM0RlbnVtIHRhcmdldCwgV0dD M0RzaXplaSBudW1BdHRhY2htZW50cywgY29uc3QgV0dDM0RlbnVtKiBhdHRhY2htZW50cykgeyB9 Cg==