Add UpdatableTexture class, which allows texture updater to allocate tile specific resources and paint tiles separately. Add LayerTextureUpdaterBitmap class that implements texture updates by recording drawing commands to a SkPicture and playing them back to each tile individually.
Created attachment 113362 [details] Patch
Created attachment 113389 [details] Patch
Comment on attachment 113389 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=113389&action=review Neat work! I have a ton of questions... mainly, I'm trying to educate myself here rather than push you in a particular direction, so I hope they're not too annoying.... The UpdatableTexture class feels somewhat similar to the LayerPainter class, except that the LayerPainter gets called multiple times per layer and doesn't know about the overall layer damage. If we told a LayerPainter up-front the union of rects that we were about to call paint on, would we be able to just build on LayerPainter for painting layers into SkPictures? Did you give any thought into making this two patches? patch 1, a no-features-added refactoring that introduces the changes necessary to allow the feature [e.g. the UpdatableTexture or whatever you settle on] and patch 2, the paint->skpicture;skpicture->texture patch? It might help us see the shape of things more-easily that way as well as deal with regressions.... I'm not insisting, really, just thinking out loud. Can we take this opportunity to move some of these updater classes into files that match their class name? One class per file is our general convention except for things like Foo/FooClient. Is the idea here that "update" is the word for going from an SkPicture to a texture? Can we call that rasterization? Or tile-filling? Tile Rasterization? TilePainting versus Layer Painting? Just throwing out ideas... whatever word we settle on, lets make sure we normalize the codebase to that terminology and put comments liberally on the new code to explain the word. Any thoughts on how to test this stuff without having to instantiate large portions of webkit and/or skia? Not in the short term, per se, but overall. We don't have a test strategy for this stuff and I think we should. > Source/WebCore/platform/graphics/chromium/ImageLayerChromium.cpp:49 > +class ImageTexture : public UpdatableTexture { ImageUpdatableTexture? I personally think its good to be able to tell the ancestery of a class by its name. > Source/WebCore/platform/graphics/chromium/ImageLayerChromium.cpp:66 > +}; Personally, I think you can put the implementations inline since this is in the cpp. Ping jamesr on chat if you want to verify that... > Source/WebCore/platform/graphics/chromium/LayerTextureUpdater.h:44 > +class UpdatableTexture : public RefCounted<UpdatableTexture> { Standalone file? Comment on the overall class explaining its "role" and purpose? > Source/WebCore/platform/graphics/chromium/LayerTextureUpdater.h:49 > + virtual void prepareRect(const IntRect& sourceRect) = 0; Comments on these functions? > Source/WebCore/platform/graphics/chromium/LayerTextureUpdaterCanvas.cpp:45 > +#include "skia/ext/platform_canvas.h" Is this the right way to do skia includes? I'm not saying its wrong, it just sorta looks odd to my eyes.
> Did you give any thought into making this two patches? ... I just realized that this is a split-out portion of a previous patch. Apologies if this feels like cruel and unusual punishment. :/
(In reply to comment #3) > (From update of attachment 113389 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=113389&action=review > > Neat work! I have a ton of questions... mainly, I'm trying to educate myself here rather than push you in a particular direction, so I hope they're not too annoying.... > > The UpdatableTexture class feels somewhat similar to the LayerPainter class, except that the LayerPainter gets called multiple times per layer and doesn't know about the overall layer damage. If we told a LayerPainter up-front the union of rects that we were about to call paint on, would we be able to just build on LayerPainter for painting layers into SkPictures? The main purpose of the UpdatableTexture class is to allow the texture updater to create per-tile resources. E.g. per-tile raster buffers for LayerTextureUpdaterBitmap or mapped shared memory buffers for LayerTextureUpdaterSharedMemory. I think this belongs to the LayerTextureUpdater class and not the LayerPainter class. LayerPainter is currently not involved in graphics context creation, which is how skpicture redirection is accomplished. > > Did you give any thought into making this two patches? patch 1, a no-features-added refactoring that introduces the changes necessary to allow the feature [e.g. the UpdatableTexture or whatever you settle on] and patch 2, the paint->skpicture;skpicture->texture patch? It might help us see the shape of things more-easily that way as well as deal with regressions.... I'm not insisting, really, just thinking out loud. I'd prefer that too. I'm generally in favor for making patches as small as possible even when that means a bit more overhead in the review process. > > Can we take this opportunity to move some of these updater classes into files that match their class name? One class per file is our general convention except for things like Foo/FooClient. The UpdatableTexture classes are so tightly coupled with corresponding LayerTextureUpdater classes that I don't think it makes sense to split them up into different files. Moving the different LayerTextureUpdater classes into separate files is a great idea though. > > Is the idea here that "update" is the word for going from an SkPicture to a texture? Can we call that rasterization? Or tile-filling? Tile Rasterization? TilePainting versus Layer Painting? Just throwing out ideas... whatever word we settle on, lets make sure we normalize the codebase to that terminology and put comments liberally on the new code to explain the word. "Update" is still the same as before, going from rasterized contents to texture. It's the texture upload. "Prepare" is still where all rasterization happens. So this patch doesn't introduce any changes in the terminology here. The follow up patch that adds support for drawing directly to mapped shared memory will need an "Allocate" step and it might makes sense to change these terms and document them properly at that point. I'm intentionally avoiding that in this patch. > > Any thoughts on how to test this stuff without having to instantiate large portions of webkit and/or skia? Not in the short term, per se, but overall. We don't have a test strategy for this stuff and I think we should. I agree but I haven't thought much about it. > > > Source/WebCore/platform/graphics/chromium/ImageLayerChromium.cpp:49 > > +class ImageTexture : public UpdatableTexture { > > ImageUpdatableTexture? I personally think its good to be able to tell the ancestery of a class by its name. Yes, ImageUpdatableTexture would follow the convention used for LayerTextureUpdater naming better. > > > Source/WebCore/platform/graphics/chromium/ImageLayerChromium.cpp:66 > > +}; > > Personally, I think you can put the implementations inline since this is in the cpp. Ping jamesr on chat if you want to verify that... ImageUpdatableTexture::updateRect requires the full declaration of ImageLayerTextureUpdater and ImageLayerTextureUpdater::createTexture requires the full declaration of ImageUpdatableTexture so one of those functions can't be inlined. I arbitrarily picked ImageUpdatableTexture::updateRect in this case. > > > Source/WebCore/platform/graphics/chromium/LayerTextureUpdater.h:44 > > +class UpdatableTexture : public RefCounted<UpdatableTexture> { > > Standalone file? As mentioned above. I think UpdatableTexture classes are so closely coupled with LayerTextureUpdater classes that we're better off keeping these in the same file. > > Comment on the overall class explaining its "role" and purpose? Sure. > > > Source/WebCore/platform/graphics/chromium/LayerTextureUpdater.h:49 > > + virtual void prepareRect(const IntRect& sourceRect) = 0; > > Comments on these functions? This is just LayerTextureUpdater::updateTextureRect moved to the UpdatableTexture class. I intentionally didn't add any comments for functions that don't already have comments. I can add some comments here if you think that would be useful? > > > Source/WebCore/platform/graphics/chromium/LayerTextureUpdaterCanvas.cpp:45 > > +#include "skia/ext/platform_canvas.h" > > Is this the right way to do skia includes? I'm not saying its wrong, it just sorta looks odd to my eyes. This is the way Source/WebCore/platform/graphics/skia/PlatformContextSkia.cpp does it.
Re: patch direction. I see where you're going with UpdatableTexture. I think that's a great way to store per-tile data, since we already have a custom tile type on the TiledLayerChromium. Two minor quibbles: (1) I don't want to keep around per-tile pixel buffers. You should probably clean them up during upload. I could imagine you'd probably have to do something analogous for the shared memory hand-off case. (2) It should probably be documented somewhere that createTextureUpdater() should always return the same kind of texture updater and a texture updater should always return the same kind of tile class. Otherwise, you'd probably need to clear all tiles when changing layer tree hosts and creating a new updater? It just seems a little bit sloppy, and maybe some documentation might be the answer here. Re: cleanup. I think there are a couple of cleanup issues that have been in the back of our heads for a while in this part of the code. Sorry that you've sort of stepped in them, but that's how it goes. I'll try to call them explicitly out here, and we can decide what to do or where we're going. * Unclear class naming (PlatformCanvas vs Canvas vs Bitmap vs SkPicture). I couldn't tell you what the difference between a canvas and bitmap was without looking at the code. The fact that we have all these derived updaters and derived texture classes here doesn't make this any easier to keep it all in your head. Also, minor nit for me is that class names are suffixed instead of prefixed (BitmapLayerUpdater vs. LayerUpdaterBitmap). * Texture updaters aren't in separate files. This is pretty trivial to fix, but maybe should wait until we discuss the class naming? * Unclear function naming (prepare/update vs. paint/raster/allocate/upload). I agree this could wait for another patch if you're adding an allocate step in there. I realize that naming discussions can end up being a bikeshed problem, but I find this code non-obvious every time I look at it. It'd be nice to have some names that give better hints as to what's going on. I'm open to any suggestions. :) > > Any thoughts on how to test this stuff without having to instantiate large portions of webkit and/or skia? Not in the short term, per se, but overall. We don't have a test strategy for this stuff and I think we should. > > I agree but I haven't thought much about it. My thought on this is that we need to break testing into two different parts. We could mock out our own LayerTextureUpdater and make sure that the right calls are being made from TiledLayerChromium. Then, we could test individual texture updaters and make sure that when calls are made on them, the right pixels get put into buffers. For the WebKit paint path, we could mock out a GraphicsContext or LayerPainterChromium. I don't see any way around the Skia dependency though. > > > Source/WebCore/platform/graphics/chromium/ImageLayerChromium.cpp:49 > > > +class ImageTexture : public UpdatableTexture { > > > > ImageUpdatableTexture? I personally think its good to be able to tell the ancestery of a class by its name. > > Yes, ImageUpdatableTexture would follow the convention used for LayerTextureUpdater naming better. How UpdatableTexture is being used looks a little strange to me. In some cases you store the updater on it. In some cases you don't (where it could be used by multiple updaters). Also, don't PlatformCanvasUpdatableTexture, CanvasUpdatableTexture, and ImageUpdatableTexture just thunk through? It really doesn't do anything other than just be a ManagedTexture wrapper. One suggestion might be to invert the flow here and encapsulate the update logic a little bit more in the texture updater itself. Rather than having TiledLayerChromium interact with the UpdatableTexture via tile->texture()->prepareRect(), maybe it should call through the updater, like we're already doing on a per-tile basis for upload. This would eliminate some of these UpdatableTexture-derived classes which only exist to hold a ManagedTexture with a different updater, and the updaters could just use ManagedTexture directly rather than a derived class.
(In reply to comment #6) > Re: patch direction. > > I see where you're going with UpdatableTexture. I think that's a great way to store per-tile data, since we already have a custom tile type on the TiledLayerChromium. Two minor quibbles: > > (1) I don't want to keep around per-tile pixel buffers. You should probably clean them up during upload. I could imagine you'd probably have to do something analogous for the shared memory hand-off case. This is a simple change. The benefit of the current code is that the per-tile buffer is only reallocated when the size has changed but the performance improvement from that might not be significant. > > (2) It should probably be documented somewhere that createTextureUpdater() should always return the same kind of texture updater and a texture updater should always return the same kind of tile class. Otherwise, you'd probably need to clear all tiles when changing layer tree hosts and creating a new updater? It just seems a little bit sloppy, and maybe some documentation might be the answer here. Each layer could potentially use a different texture updater. There's nothing in the design that prevents that afaik. Also nothing that prevents a texture updater from returning different tile classes if it would ever make sense to that. You're right that we have to clear all tiles when changing texture updater but the current code seems to do that properly afaik. Some documentation here would definitely not hurt. > > Re: cleanup. > > I think there are a couple of cleanup issues that have been in the back of our heads for a while in this part of the code. Sorry that you've sort of stepped in them, but that's how it goes. I'll try to call them explicitly out here, and we can decide what to do or where we're going. > > * Unclear class naming (PlatformCanvas vs Canvas vs Bitmap vs SkPicture). I couldn't tell you what the difference between a canvas and bitmap was without looking at the code. The fact that we have all these derived updaters and derived texture classes here doesn't make this any easier to keep it all in your head. Also, minor nit for me is that class names are suffixed instead of prefixed (BitmapLayerUpdater vs. LayerUpdaterBitmap). > > * Texture updaters aren't in separate files. This is pretty trivial to fix, but maybe should wait until we discuss the class naming? Yes, the naming here could definitely be improved and I'd prefer to wait with moving the updaters to separate files until we agree on the naming. > > * Unclear function naming (prepare/update vs. paint/raster/allocate/upload). I agree this could wait for another patch if you're adding an allocate step in there. > > I realize that naming discussions can end up being a bikeshed problem, but I find this code non-obvious every time I look at it. It'd be nice to have some names that give better hints as to what's going on. I'm open to any suggestions. :) > > > > Any thoughts on how to test this stuff without having to instantiate large portions of webkit and/or skia? Not in the short term, per se, but overall. We don't have a test strategy for this stuff and I think we should. > > > > I agree but I haven't thought much about it. > > My thought on this is that we need to break testing into two different parts. We could mock out our own LayerTextureUpdater and make sure that the right calls are being made from TiledLayerChromium. Then, we could test individual texture updaters and make sure that when calls are made on them, the right pixels get put into buffers. For the WebKit paint path, we could mock out a GraphicsContext or LayerPainterChromium. I don't see any way around the Skia dependency though. ok, sounds good to me. > > > > > Source/WebCore/platform/graphics/chromium/ImageLayerChromium.cpp:49 > > > > +class ImageTexture : public UpdatableTexture { > > > > > > ImageUpdatableTexture? I personally think its good to be able to tell the ancestery of a class by its name. > > > > Yes, ImageUpdatableTexture would follow the convention used for LayerTextureUpdater naming better. > > How UpdatableTexture is being used looks a little strange to me. In some cases you store the updater on it. In some cases you don't (where it could be used by multiple updaters). Also, don't PlatformCanvasUpdatableTexture, CanvasUpdatableTexture, and ImageUpdatableTexture just thunk through? It really doesn't do anything other than just be a ManagedTexture wrapper. > > One suggestion might be to invert the flow here and encapsulate the update logic a little bit more in the texture updater itself. Rather than having TiledLayerChromium interact with the UpdatableTexture via tile->texture()->prepareRect(), maybe it should call through the updater, like we're already doing on a per-tile basis for upload. This would eliminate some of these UpdatableTexture-derived classes which only exist to hold a ManagedTexture with a different updater, and the updaters could just use ManagedTexture directly rather than a derived class. I considered this before implementing the current patch. I decided to add the UpdatableTexture class and have the texture updater create a derived class based on it instead of ManagedTexture to avoid casts from ManagedTexture to derived class in texture updater. E.g if we kept the current LayerTextureUpdater::updateTextureRect instead of adding UpdatableTexture::updateRect. LayerTextureUpdaterBitmap::updateTextureRect would look something like this: void LayerTextureUpdaterBitmap::updateTextureRect(GraphicsContext3D* context, TextureAllocator* allocator, ManagedTexture* texture, const IntRect& sourceRect, const IntRect& destRect) { BitmapManagedTexture* bitmapManagedTexture = static_cast<BitmapManagedTexture*> (texture); texture->bindTexture(context, allocator); m_texSubImage.upload(bitmapManagedTexture->pixels(), destRect, destRect, destRect, texture->format(), context); } which I think is bad not just because it looks ugly. If you know of a cleaner way to do this please let me know.
How does this sounds as a way to move forward with this patch? - Change name of ImageTexture and such to ImageUpdatableTexture. - Add some comments explaining the purpose of UpdatableTexture class. - Agree on names for texture updater classes. The shared memory texture updater is moved to a follow up patch. Related to the texture updater naming: I used LayerTextureUpdaterPlatformCanvas as class name because that's the only updater that makes use of the PlatformCanvas class. Maybe it's better to include the fact that this updater is rasterizing to a large buffer and copying out of it to update each tile texture. LayerTextureUpdaterSubImage? LayerTextureUpdaterBitmap is I guess not the best name. It was used as we already had a texture updater with this name and it's neither using shared memory, a platform canvas or a frame buffer object. Maybe LayerTextureUpdaterSkiaBitmap and LayerTextureUpdaterSkPicture -> LayerTextureUpdaterSkia, LayerTextureUpdaterSharedMemory -> LayerTextureUpdaterSkiaSharedMemory, LayerTextureUpdaterFrameBuffer -> LayerTextureUpdaterSkiaFrameBufferObject We would end up with something like this: LayerTextureUpdaterPlatformCanvas (or LayerTextureUpdaterSubImage) LayerTextureUpdaterSkia LayerTextureUpdaterSkiaBitmap LayerTextureUpdaterSkiaSharedMemory LayerTextureUpdaterSkiaFrameBufferObject is that better?
(In reply to comment #7) > (In reply to comment #6) > > How UpdatableTexture is being used looks a little strange to me. In some cases you store the updater on it. In some cases you don't (where it could be used by multiple updaters). Also, don't PlatformCanvasUpdatableTexture, CanvasUpdatableTexture, and ImageUpdatableTexture just thunk through? It really doesn't do anything other than just be a ManagedTexture wrapper. > > > > One suggestion might be to invert the flow here and encapsulate the update logic a little bit more in the texture updater itself. Rather than having TiledLayerChromium interact with the UpdatableTexture via tile->texture()->prepareRect(), maybe it should call through the updater, like we're already doing on a per-tile basis for upload. This would eliminate some of these UpdatableTexture-derived classes which only exist to hold a ManagedTexture with a different updater, and the updaters could just use ManagedTexture directly rather than a derived class. > > I considered this before implementing the current patch. I decided to add the UpdatableTexture class and have the texture updater create a derived class based on it instead of ManagedTexture to avoid casts from ManagedTexture to derived class in texture updater. E.g if we kept the current LayerTextureUpdater::updateTextureRect instead of adding UpdatableTexture::updateRect. LayerTextureUpdaterBitmap::updateTextureRect would look something like this: > > void LayerTextureUpdaterBitmap::updateTextureRect(GraphicsContext3D* context, TextureAllocator* allocator, ManagedTexture* texture, const IntRect& sourceRect, const IntRect& destRect) > { > BitmapManagedTexture* bitmapManagedTexture = static_cast<BitmapManagedTexture*> (texture); > texture->bindTexture(context, allocator); > m_texSubImage.upload(bitmapManagedTexture->pixels(), destRect, destRect, destRect, texture->format(), context); > } > > which I think is bad not just because it looks ugly. If you know of a cleaner way to do this please let me know. I think this approach would probably be ok in practice, but I absolutely agree that the static_cast there just seems like a bad idea. On the other hand, the current approach has raw pointers to texture updaters that could be stale. Those seem about equivalently bad to me. I think I'm just balking at this idea that we've got one class hierarchy to handle painting and uploading and that we somehow need a second parallel hierarchy to store per-tile data when handling that. And, on top of that, we're going to be using at most like two pieces of each hierarchy during any given run, and texture types and updater types are never mixed and matched. It just seems like there should be a better way to do this. I think it just seems like a lot of code to write just to add virtualized per-tile data and a few function calls. Thinking out loud here: Option 1) Store per-tile data externally. There's only one updater that needs this, and on average it'll probably have no more than a screenfull of tiles. A vector or hashmap of per-tile data could be cleaner than all of this extra scaffolding for every updater type. Option 2) Texture updaters could own the tiler and explicitly manage tile lifetimes. This is somewhat of a crazy suggestion, but it would mean that they could be assured that every tile was of the type that they intended it to be, rather than requiring the derived UpdatableTile types. In some ways, the updaters are already responsible for creating tiles, so this isn't that weird. This would also fix up any explicit issues where the updater got recreated but the tiles were using the old updater (or were of a type created by an old updater type). There'd still be a static_cast, (due to CCLayerTilingData not being templatized), but I'd be ok because we'd know that there'd be a single owner responsible for what types are being stored. Do either of those sound more palatable to you? Were there other options you considered?
Its Enne's call on the overall issues, but one question I have: CCProxy CCThreadProxy LayerChromium CanvasLayerChromium Subclass name goes on front. Why does LTU hierarchy reverse this? > We would end up with something like this: > LayerTextureUpdaterPlatformCanvas (or LayerTextureUpdaterSubImage) > LayerTextureUpdaterSkia > LayerTextureUpdaterSkiaBitmap > LayerTextureUpdaterSkiaSharedMemory > LayerTextureUpdaterSkiaFrameBufferObject > > is that better?
(In reply to comment #9) > (In reply to comment #7) > > (In reply to comment #6) > > > How UpdatableTexture is being used looks a little strange to me. In some cases you store the updater on it. In some cases you don't (where it could be used by multiple updaters). Also, don't PlatformCanvasUpdatableTexture, CanvasUpdatableTexture, and ImageUpdatableTexture just thunk through? It really doesn't do anything other than just be a ManagedTexture wrapper. > > > > > > One suggestion might be to invert the flow here and encapsulate the update logic a little bit more in the texture updater itself. Rather than having TiledLayerChromium interact with the UpdatableTexture via tile->texture()->prepareRect(), maybe it should call through the updater, like we're already doing on a per-tile basis for upload. This would eliminate some of these UpdatableTexture-derived classes which only exist to hold a ManagedTexture with a different updater, and the updaters could just use ManagedTexture directly rather than a derived class. > > > > I considered this before implementing the current patch. I decided to add the UpdatableTexture class and have the texture updater create a derived class based on it instead of ManagedTexture to avoid casts from ManagedTexture to derived class in texture updater. E.g if we kept the current LayerTextureUpdater::updateTextureRect instead of adding UpdatableTexture::updateRect. LayerTextureUpdaterBitmap::updateTextureRect would look something like this: > > > > void LayerTextureUpdaterBitmap::updateTextureRect(GraphicsContext3D* context, TextureAllocator* allocator, ManagedTexture* texture, const IntRect& sourceRect, const IntRect& destRect) > > { > > BitmapManagedTexture* bitmapManagedTexture = static_cast<BitmapManagedTexture*> (texture); > > texture->bindTexture(context, allocator); > > m_texSubImage.upload(bitmapManagedTexture->pixels(), destRect, destRect, destRect, texture->format(), context); > > } > > > > which I think is bad not just because it looks ugly. If you know of a cleaner way to do this please let me know. > > I think this approach would probably be ok in practice, but I absolutely agree that the static_cast there just seems like a bad idea. On the other hand, the current approach has raw pointers to texture updaters that could be stale. Those seem about equivalently bad to me. > > I think I'm just balking at this idea that we've got one class hierarchy to handle painting and uploading and that we somehow need a second parallel hierarchy to store per-tile data when handling that. And, on top of that, we're going to be using at most like two pieces of each hierarchy during any given run, and texture types and updater types are never mixed and matched. It just seems like there should be a better way to do this. I think it just seems like a lot of code to write just to add virtualized per-tile data and a few function calls. I agree that having the texture updater not be the owner of per-tile data is a problem right now. > > Thinking out loud here: > > Option 1) Store per-tile data externally. > > There's only one updater that needs this, and on average it'll probably have no more than a screenfull of tiles. A vector or hashmap of per-tile data could be cleaner than all of this extra scaffolding for every updater type. I considered this. I'm not sure it would be much cleaner after adding the necessary API for releasing these updater specific resources. And I don't like adding an additional data-structure where none is needed. > > Option 2) Texture updaters could own the tiler and explicitly manage tile lifetimes. > > This is somewhat of a crazy suggestion, but it would mean that they could be assured that every tile was of the type that they intended it to be, rather than requiring the derived UpdatableTile types. In some ways, the updaters are already responsible for creating tiles, so this isn't that weird. This would also fix up any explicit issues where the updater got recreated but the tiles were using the old updater (or were of a type created by an old updater type). There'd still be a static_cast, (due to CCLayerTilingData not being templatized), but I'd be ok because we'd know that there'd be a single owner responsible for what types are being stored. The idea of making the updater own the tiler is worth considering. I haven't thought of that before. Think I prefer having virtualized per-tile functions over using static_cast though. > > Do either of those sound more palatable to you? Were there other options you considered? I think the best solution might be to have CCTextureUpdater own the per-tile specific data. Makes it harder to retain resources between updates but we should probably use something similar to the TextureManager for tracking those resources anyhow. This requires the "allocate" step that I've talked about before so I'd prefer to land something like the current patch first and then improve it in the follow up patch that adds the "allocate" step.
(In reply to comment #10) > Its Enne's call on the overall issues, but one question I have: > CCProxy > CCThreadProxy > LayerChromium > CanvasLayerChromium > > Subclass name goes on front. > > Why does LTU hierarchy reverse this? Yea, I just happen to use the existing class names as reference here. I'm happy to change the position of the subclass name at the same time. Though I'm more interested in feedback on what subclass names to use at this point. The structure of them is easy to decide. PlatformCanvasLayerTextureUpdater (or SubImageLayerTextureUpdater) SkiaLayerTextureUpdater BitmapSkiaLayerTextureUpdater SharedMemorySkiaLayerTextureUpdater FrameBufferObjectSkiaLayerTextureUpdater ? > > > We would end up with something like this: > > LayerTextureUpdaterPlatformCanvas (or LayerTextureUpdaterSubImage) > > LayerTextureUpdaterSkia > > LayerTextureUpdaterSkiaBitmap > > LayerTextureUpdaterSkiaSharedMemory > > LayerTextureUpdaterSkiaFrameBufferObject > > > > is that better?
(In reply to comment #12) > PlatformCanvasLayerTextureUpdater (or SubImageLayerTextureUpdater) > SkiaLayerTextureUpdater > BitmapSkiaLayerTextureUpdater > SharedMemorySkiaLayerTextureUpdater > FrameBufferObjectSkiaLayerTextureUpdater Maybe PlatformCanvasLayerTextureUpdater => BitmapLayerTextureUpdater? Then the difference between BitmapSkiaLayerTextureUpdater and BitmapLayerTextureUpdater would be whether you cached an SkPicture. Other than that, that above list sounds good to me. Re: other comments. I think I'm ok with the patch's direction of using derived tile classes after thinking about alternatives and not really liking any of them much better. Thanks for letting me take the time to pick through alternatives.
Created attachment 114089 [details] Patch
Created attachment 114106 [details] Patch
Latest patch uses these names and moves them into separate files: CanvasLayerTextureUpdater : public LayerTextureUpdater BitmapLayerTextureUpdater : public CanvasLayerTextureUpdater SkiaLayerTextureUpdater : public CanvasLayerTextureUpdater FrameBufferObjectSkiaLayerTextureUpdater : public SkiaLayerTextureUpdater the texture updaters derived from CanvasLayerTextureUpdater don't currently inherit the Canvas name. We should probably change that for consistency.
(In reply to comment #16) > Latest patch uses these names and moves them into separate files: > > CanvasLayerTextureUpdater : public LayerTextureUpdater > BitmapLayerTextureUpdater : public CanvasLayerTextureUpdater > SkiaLayerTextureUpdater : public CanvasLayerTextureUpdater > FrameBufferObjectSkiaLayerTextureUpdater : public SkiaLayerTextureUpdater Awesome. Thanks for all of that. I find this to be much more clear. This patch looks good to me.
Created attachment 114120 [details] Patch
(In reply to comment #17) > (In reply to comment #16) > > Latest patch uses these names and moves them into separate files: > > > > CanvasLayerTextureUpdater : public LayerTextureUpdater > > BitmapLayerTextureUpdater : public CanvasLayerTextureUpdater > > SkiaLayerTextureUpdater : public CanvasLayerTextureUpdater > > FrameBufferObjectSkiaLayerTextureUpdater : public SkiaLayerTextureUpdater > > Awesome. Thanks for all of that. I find this to be much more clear. This patch looks good to me. Great! I went ahead and added the Canvas name to all related classes in the latest patch as I didn't see any good reason for not ding that. Btw, each derived UpdatableTexture class is now a subclass of the texture updater class. I think that cleaned up things a bit and allowed me to inline all the texture updater code in ImageLayerChromium.cpp.
Loooks amazing! Thanks David!
Comment on attachment 114120 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=114120&action=review Lots of feedback of the nitpicky variety - no big changes requested. Overall this looks great. > Source/WebCore/platform/graphics/chromium/BitmapCanvasLayerTextureUpdater.h:47 > + virtual void prepareRect(const IntRect& sourceRect) { } i'm not sure why here and in other files you declare an empty override - should this function just not be declare pure virtual in the base class if some significant portion of subclasses will just override it with an empty implementation. if you really do want to provide this override, either omit the parameter name or comment it out with /* */ so we don't get unused variable warnings > Source/WebCore/platform/graphics/chromium/BitmapCanvasLayerTextureUpdater.h:56 > + static PassRefPtr<BitmapCanvasLayerTextureUpdater> create(PassOwnPtr<LayerPainterChromium>, bool useMapTexSubImage); you also should declare a virtual d'tor for this class and put its definition in the .cpp. Since this class is virtual and its base class has a virtual d'tor, this classes d'tor is virtual whether you declare it or not and if you do not declare it c++ will happily define a d'tor in the header. This sucks because the d'tor is large (since it has to invoke the d'tor for members and the base d'tor) and since it's virtual it is cannot be inlined, so it just bloats up the object size of every translation unit including this header and makes the linker do more work and use more memory dealing with lots of duplicate symbols. > Source/WebCore/platform/graphics/chromium/CanvasLayerTextureUpdater.h:42 > + explicit CanvasLayerTextureUpdater(PassOwnPtr<LayerPainterChromium>); declare d'tor here in the header, define in the .cpp please. > Source/WebCore/platform/graphics/chromium/LayerTextureUpdater.h:73 > + virtual Texture* createTexture(TextureManager*) = 0; it appears that all callers of this function stuff the return value into a RefPtr<>. If that is the case then it would be better for this function to return a PassRefPtr<> to reduce the odds of some caller forgetting to put the pointer in the correct wrapper. in general any function in WebKit that returns a pointer and is transferring some notion of ownership should return a PassOwnPtr or PassRefPtr - raw pointer return types are reserved for when ownership isn't being transferred (for example returning a pointer to a member that is owned exclusively by the object) > Source/WebCore/platform/graphics/chromium/SkiaCanvasLayerTextureUpdater.h:42 > +class SkiaCanvasLayerTextureUpdater : public CanvasLayerTextureUpdater { i think this would be better called SkPictureCanvasLayerTextureUpdater since other layer types also use skia in different ways > Source/WebCore/platform/graphics/chromium/SkiaCanvasLayerTextureUpdater.h:44 > + explicit SkiaCanvasLayerTextureUpdater(PassOwnPtr<LayerPainterChromium>); a public raw c'tor is weird - normally we only do this if we want to instantiate instances of this class on the stack or as member variables of other classes, otherwise we use a static create() of the correct smart pointer type. It seems other CanvasLayerTextureUpdater subclasses expose a create() so I'm not sure why this one is different. please declare a virtual d'tor here and define it in the .cpp > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:54 > + explicit UpdatableTile(PassRefPtr<LayerTextureUpdater::Texture> tex) : m_tex(tex) { } nit: this isn't your fault, but we don't typically abbreviate words like 'texture' to 'tex' even in one-liners like this. can you fix this since you're already here? > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:64 > + // Page-space rectangle that needs to be updated. "page" is an ambiguous word to use here - what page? the page the layer is in? Do you mean content space? > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:67 > + RefPtr<LayerTextureUpdater::Texture> m_tex; similarly this should be m_texture, not m_tex
(In reply to comment #21) > (From update of attachment 114120 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=114120&action=review > > Lots of feedback of the nitpicky variety - no big changes requested. Overall this looks great. > > > Source/WebCore/platform/graphics/chromium/BitmapCanvasLayerTextureUpdater.h:47 > > + virtual void prepareRect(const IntRect& sourceRect) { } > > i'm not sure why here and in other files you declare an empty override - should this function just not be declare pure virtual in the base class if some significant portion of subclasses will just override it with an empty implementation. This was done to be consistent with LayerTextureUpdater::prepareToUpdate. Making both LayerTextureUpdater::prepareToUpdate and LayerTextureUpdater::Texture::prepareRect not declared pure virtual. > > if you really do want to provide this override, either omit the parameter name or comment it out with /* */ so we don't get unused variable warnings Adding /* */ to a few unused parameters in latest patch. > > > Source/WebCore/platform/graphics/chromium/BitmapCanvasLayerTextureUpdater.h:56 > > + static PassRefPtr<BitmapCanvasLayerTextureUpdater> create(PassOwnPtr<LayerPainterChromium>, bool useMapTexSubImage); > > you also should declare a virtual d'tor for this class and put its definition in the .cpp. Since this class is virtual and its base class has a virtual d'tor, this classes d'tor is virtual whether you declare it or not and if you do not declare it c++ will happily define a d'tor in the header. This sucks because the d'tor is large (since it has to invoke the d'tor for members and the base d'tor) and since it's virtual it is cannot be inlined, so it just bloats up the object size of every translation unit including this header and makes the linker do more work and use more memory dealing with lots of duplicate symbols. Understand, thanks for the explanation. Fixing. > > > Source/WebCore/platform/graphics/chromium/CanvasLayerTextureUpdater.h:42 > > + explicit CanvasLayerTextureUpdater(PassOwnPtr<LayerPainterChromium>); > > declare d'tor here in the header, define in the .cpp please. Fixing. > > > Source/WebCore/platform/graphics/chromium/LayerTextureUpdater.h:73 > > + virtual Texture* createTexture(TextureManager*) = 0; > > it appears that all callers of this function stuff the return value into a RefPtr<>. If that is the case then it would be better for this function to return a PassRefPtr<> to reduce the odds of some caller forgetting to put the pointer in the correct wrapper. > > in general any function in WebKit that returns a pointer and is transferring some notion of ownership should return a PassOwnPtr or PassRefPtr - raw pointer return types are reserved for when ownership isn't being transferred (for example returning a pointer to a member that is owned exclusively by the object) Yea, I think I did this to avoid adding an adoptRef in each createTexture but that's obviously a bad idea. Fixing. > > > Source/WebCore/platform/graphics/chromium/SkiaCanvasLayerTextureUpdater.h:42 > > +class SkiaCanvasLayerTextureUpdater : public CanvasLayerTextureUpdater { > > i think this would be better called SkPictureCanvasLayerTextureUpdater since other layer types also use skia in different ways Sure. > > > Source/WebCore/platform/graphics/chromium/SkiaCanvasLayerTextureUpdater.h:44 > > + explicit SkiaCanvasLayerTextureUpdater(PassOwnPtr<LayerPainterChromium>); > > a public raw c'tor is weird - normally we only do this if we want to instantiate instances of this class on the stack or as member variables of other classes, otherwise we use a static create() of the correct smart pointer type. It seems other CanvasLayerTextureUpdater subclasses expose a create() so I'm not sure why this one is different. That c'tor should have been protected. My mistake. It doesn't have a static create method the class is not instantiated on it's own. It needs to be derived. > > please declare a virtual d'tor here and define it in the .cpp > > > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:54 > > + explicit UpdatableTile(PassRefPtr<LayerTextureUpdater::Texture> tex) : m_tex(tex) { } > > nit: this isn't your fault, but we don't typically abbreviate words like 'texture' to 'tex' even in one-liners like this. can you fix this since you're already here? Sure, no prob. > > > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:64 > > + // Page-space rectangle that needs to be updated. > > "page" is an ambiguous word to use here - what page? the page the layer is in? Do you mean content space? I used "page" to be consistent with the "Calculate page-space rectangle to copy from" comment in prepareToUpdate. Changing to content-space in both places. > > > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:67 > > + RefPtr<LayerTextureUpdater::Texture> m_tex; > > similarly this should be m_texture, not m_tex np.
Created attachment 114338 [details] Patch
Comment on attachment 114338 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=114338&action=review > Source/WebCore/platform/graphics/chromium/BitmapLayerTextureUpdater.cpp:63 > +LayerTextureUpdater::Texture* BitmapCanvasLayerTextureUpdater::createTexture(TextureManager* manager) > +{ > + return new Texture(this, ManagedTexture::create(manager)); > +} this signature doesn't match the .h - how does this compile?
Created attachment 114365 [details] Patch
(In reply to comment #24) > (From update of attachment 114338 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=114338&action=review > > > Source/WebCore/platform/graphics/chromium/BitmapLayerTextureUpdater.cpp:63 > > +LayerTextureUpdater::Texture* BitmapCanvasLayerTextureUpdater::createTexture(TextureManager* manager) > > +{ > > + return new Texture(this, ManagedTexture::create(manager)); > > +} > > this signature doesn't match the .h - how does this compile? Oops, that's an old file that shouldn't be part of this patch. Fixed in latest patch.
Comment on attachment 114365 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=114365&action=review Very very close! Just two more nits. > Source/WebCore/platform/graphics/chromium/LayerTextureUpdater.h:52 > + virtual void prepareRect(const IntRect& sourceRect) { }; you don't need a semicolon at the end of this line this will have the unused parameter issue - don't name the parameter or put the parameter name in comments like so: virtual void prepareRect(const IntRect& /* sourceRect */) { } or just make it pure virtual (that might make most sense, this class isn't instantiable anyway) > Source/WebCore/platform/graphics/chromium/LayerTextureUpdater.h:81 > + virtual void prepareToUpdate(const IntRect& contentRect, const IntSize& tileSize, int borderTexels) { }; no semicolon at end of line
Created attachment 114419 [details] Patch
Comment on attachment 114419 [details] Patch Clearing flags on attachment: 114419 Committed r99813: <http://trac.webkit.org/changeset/99813>
All reviewed patches have been landed. Closing bug.
FYI r99813 added + const IntPoint anchor = sourceRect.location(); to TiledLayerChromium.cpp, but that variable is then unused, triggering a build error on gcc 4.6.
(In reply to comment #31) > FYI r99813 added > + const IntPoint anchor = sourceRect.location(); > to TiledLayerChromium.cpp, but that variable is then unused, triggering a build error on gcc 4.6. Sorry about that. Here's a fix: https://bugs.webkit.org/show_bug.cgi?id=72199
Reverted in http://trac.webkit.org/changeset/100392 due to http://code.google.com/p/chromium/issues/detail?id=104014
Created attachment 115648 [details] Patch
*** Bug 72333 has been marked as a duplicate of this bug. ***
Created attachment 116000 [details] Patch
Attachment 114419 [details] landed before but was reverted because of this issue: https://bugs.webkit.org/show_bug.cgi?id=72630 the changes in the new patch compared to the patch that landed before are mostly from rebaseing onto trunk. Here are the interesting parts of the diff between the patches: diff --git a/Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp b/Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp @@ -1348,9 +1375,19 @@ }; TiledLayerChromium::TiledLayerChromium(CCLayerDelegate* delegate) -@@ -167,17 +170,12 @@ void TiledLayerChromium::updateCompositorResources(GraphicsContext3D* context, C - else if (!tile->dirty()) - continue; +@@ -173,22 +176,19 @@ void TiledLayerChromium::updateCompositorResources(GraphicsContext3D* context, C + for (int j = top; j <= bottom; ++j) { + for (int i = left; i <= right; ++i) { + UpdatableTile* tile = tileAt(i, j); ++ ++ // Required tiles are created in prepareToUpdate(). A tile should ++ // never be removed between the call to prepareToUpdate() and the ++ // call to updateCompositorResources(). + if (!tile) +- tile = createTile(i, j); +- else if (!tile->dirty()) +- continue; ++ CRASH(); - // Calculate page-space rectangle to copy from. - IntRect sourceRect = m_tiler->tileContentRect(tile); We should never have to create tiles here and even if we would, things would break. @@ -1381,11 +1418,12 @@ GLC(context, context->texParameteri(GraphicsContext3D::TEXTURE_2D, GraphicsContext3D::TEXTURE_MAG_FILTER, filter)); - updater.append(tile->texture(), textureUpdater(), sourceRect, destRect); +- tile->clearDirty(); + updater.append(tile->texture(), sourceRect, destRect); - tile->clearDirty(); } } prepareToUpdate is a better place to call clear dirty. @@ -1420,7 +1458,20 @@ } } } -@@ -346,10 +344,10 @@ void TiledLayerChromium::prepareToUpdate(const IntRect& contentRect) +@@ -334,6 +333,12 @@ void TiledLayerChromium::prepareToUpdate(const IntRect& contentRect) + + m_skipsDraw = false; + ++ // Reset m_updateRect for all tiles. ++ for (CCLayerTilingData::TileMap::const_iterator iter = m_tiler->tiles().begin(); iter != m_tiler->tiles().end(); ++iter) { ++ UpdatableTile* tile = static_cast<UpdatableTile*>(iter->second.get()); ++ tile->m_updateRect = IntRect(); ++ } ++ + if (contentRect.isEmpty()) { + m_requestedUpdateRect = IntRect(); + return; +@@ -357,10 +362,10 @@ void TiledLayerChromium::prepareToUpdate(const IntRect& contentRect) if (!tile) tile = createTile(i, j); Reset m_updateRect before starting a new update. @@ -1433,28 +1484,33 @@ m_skipsDraw = true; cleanupResources(); return; -@@ -374,6 +372,31 @@ void TiledLayerChromium::prepareToUpdate(const IntRect& contentRect) +@@ -385,6 +390,36 @@ void TiledLayerChromium::prepareToUpdate(const IntRect& contentRect) // so we grab a local reference here to hold the updater alive until the paint completes. RefPtr<LayerTextureUpdater> protector(textureUpdater()); - textureUpdater()->prepareToUpdate(m_paintRect, m_tiler->tileSize(), m_tiler->hasBorderTexels()); -+ + textureUpdater()->prepareToUpdate(m_paintRect, m_tiler->tileSize(), m_tiler->hasBorderTexels(), contentsScale()); + m_tiler->contentRectToTileIndices(m_requestedUpdateRect, left, top, right, bottom); + for (int j = top; j <= bottom; ++j) { + for (int i = left; i <= right; ++i) { + UpdatableTile* tile = tileAt(i, j); ++ ++ // Tiles are created before prepareToUpdate() is called. + if (!tile) -+ tile = createTile(i, j); -+ else if (!tile->dirty()) ++ CRASH(); ++ ++ if (!tile->dirty()) + continue; + + // Calculate content-space rectangle to copy from. + IntRect sourceRect = m_tiler->tileContentRect(tile); -+ const IntPoint anchor = sourceRect.location(); + sourceRect.intersect(m_tiler->layerRectToContentRect(tile->m_dirtyLayerRect)); + // Paint rect not guaranteed to line up on tile boundaries, so + // make sure that sourceRect doesn't extend outside of it. + sourceRect.intersect(m_paintRect); + ++ // updateCompositorResources() uses m_updateRect to determine ++ // the tiles to update so we can clear the dirty rectangle here. ++ tile->clearDirty(); ++ + tile->m_updateRect = sourceRect; + if (sourceRect.isEmpty()) + continue; All required tiles are created before prepareToUpdate is called. Remove unused anchor variable (https://bugs.webkit.org/show_bug.cgi?id=72199). Clear dirty rectangle here.
Created attachment 116291 [details] Patch
Latest patch just removes unused ref counting support from the LayerTextureUpdater::Texture class.
Created attachment 117014 [details] Patch
Comment on attachment 117014 [details] Patch Attachment 117014 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10709076 New failing tests: svg/transforms/text-with-pattern-with-svg-transform.svg
Comment on attachment 117014 [details] Patch This looks great - R=me. I'm sorry this took so long to get to, I've been swamped. Flip cq? whenever you're ready to land this.
Comment on attachment 117014 [details] Patch Clearing flags on attachment: 117014 Committed r101600: <http://trac.webkit.org/changeset/101600>