Bug 60719 - Split canvas from LayerTilerChromium
Summary: Split canvas from LayerTilerChromium
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Alok Priyadarshi
URL:
Keywords:
Depends on: 60983
Blocks: 56749 60934
  Show dependency treegraph
 
Reported: 2011-05-12 12:52 PDT by Alok Priyadarshi
Modified: 2011-05-18 16:53 PDT (History)
7 users (show)

See Also:


Attachments
proposed patch (56.90 KB, patch)
2011-05-13 10:34 PDT, Alok Priyadarshi
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
proposed patch (56.64 KB, patch)
2011-05-13 14:14 PDT, Alok Priyadarshi
no flags Details | Formatted Diff | Diff
proposed patch (56.59 KB, patch)
2011-05-13 14:36 PDT, Alok Priyadarshi
jamesr: review-
Details | Formatted Diff | Diff
proposed patch (57.13 KB, patch)
2011-05-16 08:46 PDT, Alok Priyadarshi
jamesr: review-
Details | Formatted Diff | Diff
proposed patch (57.19 KB, patch)
2011-05-16 14:49 PDT, Alok Priyadarshi
no flags Details | Formatted Diff | Diff
Fix for the image assertion issues. (1.68 KB, patch)
2011-05-17 19:47 PDT, Adrienne Walker
no flags Details | Formatted Diff | Diff
Patch (36.24 KB, patch)
2011-05-18 15:26 PDT, Adrienne Walker
no flags Details | Formatted Diff | Diff
Patch (58.30 KB, patch)
2011-05-18 15:49 PDT, Adrienne Walker
jamesr: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alok Priyadarshi 2011-05-12 12:52:48 PDT
LayerTilerChromium paints into a bitmap canvas and uploads bitmap pixels to update tile textures. The task of painting and updating textures should be delegated to a custom object so that other types of canvases and update mechanisms can be used.
Comment 1 Alok Priyadarshi 2011-05-13 10:34:08 PDT
Created attachment 93479 [details]
proposed patch
Comment 2 WebKit Review Bot 2011-05-13 12:03:39 PDT
Comment on attachment 93479 [details]
proposed patch

Attachment 93479 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8698132
Comment 3 Alok Priyadarshi 2011-05-13 14:14:55 PDT
Created attachment 93508 [details]
proposed patch

fixed warning on cr-linux
Comment 4 Alok Priyadarshi 2011-05-13 14:36:24 PDT
Created attachment 93513 [details]
proposed patch

fixed merge issues. grrr
Comment 5 Adrienne Walker 2011-05-13 16:26:18 PDT
Comment on attachment 93513 [details]
proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=93513&action=review

Awesome, thanks for all this.  I have a bunch of small nits for this patch and some thoughts about where we're going to have to clean up some of this code that you're touching in the future.

> Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:-114
>      LayerChromium::setLayerRenderer(layerRenderer);
>      createTilerIfNeeded();
> -    m_tiler->setLayerRenderer(layerRenderer);

This will not set the layer renderer properly if it changes, because the tiler will already have been created.

> Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:121
> +PassOwnPtr<LayerTextureUpdater> ContentLayerChromium::createTextureUpdater()

I wish that the base version of createTextureUpdater didn't live on ContentLayerChromium, but that can wait until the ImageLayerChromium / ContentLayerChromium split.

> Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:123
> +    LayerPainterChromium* painter = new ContentLayerPainter(m_owner);

adoptPtr on the same line as new.

> Source/WebCore/platform/graphics/chromium/ImageLayerChromium.cpp:76
> +    const PlatformImage& m_image;

The const PlatformImage& member variable is a little weird here, but I think it's the right choice for this patch.  In the future, when the compositor is on another thread, ImageLayerChromium will send an ImageLayerTextureUpdater to the compositor thread.  I think ultimately ImageLayerChromium should own ImageLayerTextureUpdater, and ImageLayerTextureUpdater should own PlatformImage.  Can you add a "// FIXME: ImageLayerTextureUpdater should own m_image"?

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:98
> -PassRefPtr<LayerRendererChromium> LayerRendererChromium::create(PassRefPtr<GraphicsContext3D> context, PassOwnPtr<TilePaintInterface> contentPaint)
> +PassRefPtr<LayerRendererChromium> LayerRendererChromium::create(PassRefPtr<GraphicsContext3D> context, PassOwnPtr<LayerPainterChromium> contentPaint)

The fact that you had to make this change only makes me feel more strongly that we need to make root layers a LayerChromium and not their own special class in some future patch.

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:130
> +    LayerTextureUpdater* textureUpdater = new LayerTextureUpdaterBitmap(m_context.get(), contentPaint, m_contextSupportsMapSub);

adoptPtr on the same line.

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.h:64
> +#if USE(SKIA)
> +class SkCanvas;
> +#endif
> +

Why is this needed?

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.h:-198
> -

Nit: don't make unnecessary whitespace changes, because it's more likely to cause merge conflicts for others.

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.h:-236
> -

Nit: same as above.

> Source/WebCore/platform/graphics/chromium/LayerTextureSubImage.cpp:63
> +    if (m_useMapTexSubImage) {

There have been a number of crashes in all of this memcpy code in the past.  Can you split this out into two helper functions like uploadWithMapTexSubImage and uploadWithTexSubImage so that we can see where the problem is in a stack trace if this does crash?

> Source/WebCore/platform/graphics/chromium/LayerTextureSubImage.h:42
> +    LayerTextureSubImage(bool useMapSubForUpload);

This should be explicit.

> Source/WebCore/platform/graphics/chromium/LayerTextureUpdater.h:41
> +    LayerTextureUpdater(GraphicsContext3D* context) : m_context(context) { }

This should be explicit.

> Source/WebCore/platform/graphics/chromium/LayerTextureUpdater.h:51
> +    virtual void prepareToUpdate(const IntRect& contentRect, const IntSize& tileSize, int borderTexels) = 0;
> +    virtual void updateTextureRect(LayerTexture*, const IntRect& sourceRect, const IntRect& destRect) = 0;

This might be bikeshedding, but for consistency's sake could we call these the same thing as the LayerTilerChromium functions, i.e. update() and upload()?

> Source/WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:307
> -            IntPoint paintOffset(sourceRect.x() - paintRect.x(), sourceRect.y() - paintRect.y());
> -            if (paintOffset.x() < 0)
> -                CRASH();
> -            if (paintOffset.y() < 0)
> -                CRASH();
> -            if (paintOffset.x() + destRect.width() > paintRect.width())
> -                CRASH();
> -            if (paintOffset.y() + destRect.height() > paintRect.height())
> -                CRASH();
> +            IntPoint paintOffset(sourceRect.x() - m_paintRect.x(), sourceRect.y() - m_paintRect.y());
> +            ASSERT((paintOffset.x() >= 0) && (paintOffset.y() >= 0));
> +            ASSERT(paintOffset.x() + destRect.width() <= m_paintRect.width());
> +            ASSERT(paintOffset.y() + destRect.height() <= m_paintRect.height());

Can you not turn these CRASH calls into ASSERT? These have failed in the past due to unexpected inputs and when that happens it causes memory overruns.  If you want to clean this up, please do so in another patch so that it'd be easily revertible.
Comment 6 Adrienne Walker 2011-05-13 16:43:28 PDT
(In reply to comment #5)
> (From update of attachment 93513 [details])

> > Source/WebCore/platform/graphics/chromium/LayerTextureSubImage.h:42
> > +    LayerTextureSubImage(bool useMapSubForUpload);
> 
> This should be explicit.

Oops.  I didn't mean this.
Comment 7 James Robinson 2011-05-13 17:43:12 PDT
Comment on attachment 93513 [details]
proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=93513&action=review

>> Source/WebCore/platform/graphics/chromium/ImageLayerChromium.cpp:76
>> +    const PlatformImage& m_image;
> 
> The const PlatformImage& member variable is a little weird here, but I think it's the right choice for this patch.  In the future, when the compositor is on another thread, ImageLayerChromium will send an ImageLayerTextureUpdater to the compositor thread.  I think ultimately ImageLayerChromium should own ImageLayerTextureUpdater, and ImageLayerTextureUpdater should own PlatformImage.  Can you add a "// FIXME: ImageLayerTextureUpdater should own m_image"?

how could this work?  The PlatformImage changes on an ImageLayerChromium, potentially every frame (consider an animated gif).  do we make a new uploader every frame?

this seems...dodgy
Comment 8 James Robinson 2011-05-13 17:57:24 PDT
(In reply to comment #7)
> (From update of attachment 93513 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=93513&action=review
> 
> >> Source/WebCore/platform/graphics/chromium/ImageLayerChromium.cpp:76
> >> +    const PlatformImage& m_image;
> > 
> > The const PlatformImage& member variable is a little weird here, but I think it's the right choice for this patch.  In the future, when the compositor is on another thread, ImageLayerChromium will send an ImageLayerTextureUpdater to the compositor thread.  I think ultimately ImageLayerChromium should own ImageLayerTextureUpdater, and ImageLayerTextureUpdater should own PlatformImage.  Can you add a "// FIXME: ImageLayerTextureUpdater should own m_image"?
> 
> how could this work?  The PlatformImage changes on an ImageLayerChromium, potentially every frame (consider an animated gif).  do we make a new uploader every frame?
> 
> this seems...dodgy

Oh, I was mistaken - the object remains the same but the contents change.  This still feels as if something is being misplaced.
Comment 9 James Robinson 2011-05-13 18:34:26 PDT
Comment on attachment 93513 [details]
proposed patch

It seems weird and fragile that for content layers the PlatformCanvas is owned by the updater, but for image layers the PlatformImage is owned by the layer and referenced by the updater.  Can you have the updater have ownership of the PlatformImage directly?
Comment 10 Alok Priyadarshi 2011-05-16 08:36:33 PDT
Comment on attachment 93513 [details]
proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=93513&action=review

>> Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:-114
>> -    m_tiler->setLayerRenderer(layerRenderer);
> 
> This will not set the layer renderer properly if it changes, because the tiler will already have been created.

Actually it does work. LayerChromium::setLayerRenderer() calls ContentLayerChromium::cleanupResources() which in turn clears the tiler. So now we are re-creating the tiler if layer-renderer changes. We were earlier doing something similar in LayerTilerChromium::reset(). Now that LayerTilerChromium owns LayerTextureUpdater which keeps reference to layer-renderer-context, I think it is cleaner to just create the updater and tiler from scratch whenever layer-renderer changes.

>> Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:121

> 
> I wish that the base version of createTextureUpdater didn't live on ContentLayerChromium, but that can wait until the ImageLayerChromium / ContentLayerChromium split.

Yes this will not be necessary after the ImageLayerChromium / ContentLayerChromium split.

>> Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:123
>> +    LayerPainterChromium* painter = new ContentLayerPainter(m_owner);
> 
> adoptPtr on the same line as new.

done

>>>> Source/WebCore/platform/graphics/chromium/ImageLayerChromium.cpp:76
>>>> +    const PlatformImage& m_image;
>>> 
>>> The const PlatformImage& member variable is a little weird here, but I think it's the right choice for this patch.  In the future, when the compositor is on another thread, ImageLayerChromium will send an ImageLayerTextureUpdater to the compositor thread.  I think ultimately ImageLayerChromium should own ImageLayerTextureUpdater, and ImageLayerTextureUpdater should own PlatformImage.  Can you add a "// FIXME: ImageLayerTextureUpdater should own m_image"?
>> 
>> how could this work?  The PlatformImage changes on an ImageLayerChromium, potentially every frame (consider an animated gif).  do we make a new uploader every frame?
>> 
>> this seems...dodgy
> 
> Oh, I was mistaken - the object remains the same but the contents change.  This still feels as if something is being misplaced.

I agree that it will be cleaner if ImageLayerTextureUpdater owned PlatformImage. But ImageLayerChromium still uses PlatformImage size to mark tiler dirty and to updateLayerSize(). I guess you could get image size from somewhere else, but I did not want to change too many things in this patch. I have added a FIXME.

>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:130
>> +    LayerTextureUpdater* textureUpdater = new LayerTextureUpdaterBitmap(m_context.get(), contentPaint, m_contextSupportsMapSub);
> 
> adoptPtr on the same line.

done

>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.h:64
>> +
> 
> Why is this needed?

Because I removed "#include PlatformCanvas.h" from some other header file. LayerRendererChromium has an SkCanvas member variable.

>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.h:-198
>> -
> 
> Nit: don't make unnecessary whitespace changes, because it's more likely to cause merge conflicts for others.

reverted

>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.h:-236
>> -
> 
> Nit: same as above.

reverted

>> Source/WebCore/platform/graphics/chromium/LayerTextureSubImage.cpp:63
>> +    if (m_useMapTexSubImage) {
> 
> There have been a number of crashes in all of this memcpy code in the past.  Can you split this out into two helper functions like uploadWithMapTexSubImage and uploadWithTexSubImage so that we can see where the problem is in a stack trace if this does crash?

done

>> Source/WebCore/platform/graphics/chromium/LayerTextureUpdater.h:41
>> +    LayerTextureUpdater(GraphicsContext3D* context) : m_context(context) { }
> 
> This should be explicit.

done

>> Source/WebCore/platform/graphics/chromium/LayerTextureUpdater.h:51
>> +    virtual void updateTextureRect(LayerTexture*, const IntRect& sourceRect, const IntRect& destRect) = 0;
> 
> This might be bikeshedding, but for consistency's sake could we call these the same thing as the LayerTilerChromium functions, i.e. update() and upload()?

For a class named LayerTextureUpdater, update should really update textures. What if we renamed LayerTilerChromium functions?

>> Source/WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:307
>> +            ASSERT(paintOffset.y() + destRect.height() <= m_paintRect.height());
> 
> Can you not turn these CRASH calls into ASSERT? These have failed in the past due to unexpected inputs and when that happens it causes memory overruns.  If you want to clean this up, please do so in another patch so that it'd be easily revertible.

reverted
Comment 11 Alok Priyadarshi 2011-05-16 08:46:51 PDT
Created attachment 93647 [details]
proposed patch

Addressed reviewer comments.
Comment 12 Adrienne Walker 2011-05-16 11:39:40 PDT
(In reply to comment #10)
> (From update of attachment 93513 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=93513&action=review
> 
> >> Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:-114
> >> -    m_tiler->setLayerRenderer(layerRenderer);
> > 
> > This will not set the layer renderer properly if it changes, because the tiler will already have been created.
> 
> Actually it does work. LayerChromium::setLayerRenderer() calls ContentLayerChromium::cleanupResources() which in turn clears the tiler.

Thanks for the explanation.  That makes perfect sense.
 
> >> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.h:64
> >> +
> > 
> > Why is this needed?
> 
> Because I removed "#include PlatformCanvas.h" from some other header file. LayerRendererChromium has an SkCanvas member variable.

Ah, ok.  I just had an assumption that LayerRendererChromium shouldn't depend on any Skia or Cg classes.

I just uploaded a patch to https://bugs.webkit.org/show_bug.cgi?id=60899 to fix this, so if that lands first, then you shouldn't need that #ifdef.

> >> +    virtual void updateTextureRect(LayerTexture*, const IntRect& sourceRect, const IntRect& destRect) = 0;
> > 
> > This might be bikeshedding, but for consistency's sake could we call these the same thing as the LayerTilerChromium functions, i.e. update() and upload()?
> 
> For a class named LayerTextureUpdater, update should really update textures. What if we renamed LayerTilerChromium functions?

Good suggestion.  That sounds fine to me to just call the LayerTilerChromium functions prepareToUpdate and updateTextureRect.  Although, I'd maybe change updateTextureRect to updateTexture because the former sounds like you're updating a rect rather than updating the texture itself.

James, do you have any naming opinions here?
Comment 13 James Robinson 2011-05-16 14:07:18 PDT
Comment on attachment 93647 [details]
proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=93647&action=review

This is looking pretty good now.  I'm still a bit uneasy about the const PlatformImage& on ImageLayerTextureUpdater, but that doesn't have to be resolved in this patch.  I don't feel strongly about the update() names.  Needs a merge and a few nits but I think we're about there.

> Source/WebCore/platform/graphics/chromium/ImageLayerChromium.cpp:73
> +        return IntRect(IntPoint(0, 0), m_image.size());

nit: you can just do IntPoint(). this does not seem like a very useful utility function

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.h:63
> +#if USE(SKIA)
> +class SkCanvas;
> +#endif

If you merge up to ToT this won't be needed any more.

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.h:156
> +    explicit LayerRendererChromium(PassRefPtr<GraphicsContext3D>, PassOwnPtr<LayerPainterChromium> contentPaint);

nit: explicit isn't needed since this c'tor take stwo arguments (you did not break this, but since you're in here anyway might as well clean it up)

> Source/WebCore/platform/graphics/chromium/LayerTextureSubImage.h:42
> +    LayerTextureSubImage(bool useMapSubForUpload);

need explicit here
Comment 14 Alok Priyadarshi 2011-05-16 14:28:14 PDT
Comment on attachment 93647 [details]
proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=93647&action=review

>> Source/WebCore/platform/graphics/chromium/ImageLayerChromium.cpp:73
>> +        return IntRect(IntPoint(0, 0), m_image.size());
> 
> nit: you can just do IntPoint(). this does not seem like a very useful utility function

used IntPoint::zero() instead.

>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.h:63
>> +#endif
> 
> If you merge up to ToT this won't be needed any more.

removed

>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.h:156
>> +    explicit LayerRendererChromium(PassRefPtr<GraphicsContext3D>, PassOwnPtr<LayerPainterChromium> contentPaint);
> 
> nit: explicit isn't needed since this c'tor take stwo arguments (you did not break this, but since you're in here anyway might as well clean it up)

done

>> Source/WebCore/platform/graphics/chromium/LayerTextureSubImage.h:42
>> +    LayerTextureSubImage(bool useMapSubForUpload);
> 
> need explicit here

done
Comment 15 Alok Priyadarshi 2011-05-16 14:49:55 PDT
Created attachment 93696 [details]
proposed patch

Main changes:
- Replaced IntPoint(0,0) with IntPoint::zero()
- Removed #ifdef(SKIA) from LayerRendererChromium.h
- explicit constructors
- Renamed LayerTilerChromium member functions: update -> prepareToUpdate and upload -> updateRect
Comment 16 Adrienne Walker 2011-05-16 15:04:09 PDT
Comment on attachment 93696 [details]
proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=93696&action=review

One small nit:

> Source/WebCore/platform/graphics/chromium/LayerTilerChromium.h:60
> +    void updateRect();

As I mentioned before, I would still like the function names to be consistent.  Can LayerTilerChromium::updateRect => LayerTilerChromium::updateTexture and LayerTextureUpdater::updateTextureRect => LayerTextureUpdater::updateTexture?
Comment 17 Alok Priyadarshi 2011-05-16 15:14:55 PDT
Comment on attachment 93696 [details]
proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=93696&action=review

>> Source/WebCore/platform/graphics/chromium/LayerTilerChromium.h:60
>> +    void updateRect();
> 
> As I mentioned before, I would still like the function names to be consistent.  Can LayerTilerChromium::updateRect => LayerTilerChromium::updateTexture and LayerTextureUpdater::updateTextureRect => LayerTextureUpdater::updateTexture?

updateTexture is fine too. But I do not think it is important for the function names to be consistent. Though they are internally doing the same thing, context is different. LayerTilerChromium is updating many textures for a dirty content-rect while LayerTextureUpdater is updating a rect within a single texture. For the clients of LayerTilerChromium, it should not matter if the tiler even uses textures. They should just be able to invalidate, update, and draw.

But if you feel strongly about this, I can make this change. I am fine either way.
Comment 18 Adrienne Walker 2011-05-16 15:21:17 PDT
(In reply to comment #17)
> (From update of attachment 93696 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=93696&action=review
> 
> >> Source/WebCore/platform/graphics/chromium/LayerTilerChromium.h:60
> >> +    void updateRect();
> > 
> > As I mentioned before, I would still like the function names to be consistent.  Can LayerTilerChromium::updateRect => LayerTilerChromium::updateTexture and LayerTextureUpdater::updateTextureRect => LayerTextureUpdater::updateTexture?
> 
> updateTexture is fine too. But I do not think it is important for the function names to be consistent. Though they are internally doing the same thing, context is different. LayerTilerChromium is updating many textures for a dirty content-rect while LayerTextureUpdater is updating a rect within a single texture. For the clients of LayerTilerChromium, it should not matter if the tiler even uses textures. They should just be able to invalidate, update, and draw.

That's reasonable.  Just leave it as-is.
Comment 19 WebKit Commit Bot 2011-05-16 15:59:01 PDT
Comment on attachment 93696 [details]
proposed patch

Clearing flags on attachment: 93696

Committed r86625: <http://trac.webkit.org/changeset/86625>
Comment 20 WebKit Commit Bot 2011-05-16 15:59:08 PDT
All reviewed patches have been landed.  Closing bug.
Comment 21 Andrew Wilson 2011-05-16 17:19:42 PDT
Rolling out because it's crashing the chromium canaries.

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpectations=true&group=%40ToT%20GPU%20Mesa%20-%20chromium.org&tests=compositing%2Fcolor-matching%2Fimage-color-matching.html

ASSERTION FAILED: imageRect().contains(contentRect)
third_party/WebKit/Source/WebCore/platform/graphics/chromium/ImageLayerChromium.cpp(60) : virtual void WebCore::ImageLayerTextureUpdater::prepareToUpdate(const WebCore::IntRect&, const WebCore::IntSize&, int)
[3429:3429:2430038712840:ERROR:process_util_posix.cc(108)] Received signal 11
Comment 22 Andrew Wilson 2011-05-16 17:21:16 PDT
Reverted r86625 for reason:

Caused failed assertion on Chromium gpu canary bots

Committed r86640: <http://trac.webkit.org/changeset/86640>
Comment 23 Adrienne Walker 2011-05-17 19:47:47 PDT
Created attachment 93859 [details]
Fix for the image assertion issues.

The above assertion was being triggered because the paint rect was larger than the image itself.  This is because in the update function the paint rect is potentially extended to tile boundaries.

There was some offline discussion between Alok and me about whether we should have the tiler store explicit layer bounds and have different sized textures for outside tiles.  I thought this might be easy, but it ultimately added a lot more complication to the tiler's drawing and recycling mechanisms, where the tiler previously had a single tile size for all textures.

After some thought, I think the best solution is to just clip any request for uploading pixels from outside the image bounds to the image bounds itself.  Other layers have no problem painting outside their bounds, so this is really just an image-specific restriction.  Intersecting with the image bounds mimics the same behavior as before this patch, but the intersection has just moved to the updater from the tiler as a result of this refactoring.

I've attached a patch which implements this suggestion and causes that failing test to pass.
Comment 24 Alok Priyadarshi 2011-05-17 20:05:53 PDT
(In reply to comment #23)
> Created an attachment (id=93859) [details]
> Fix for the image assertion issues.
> 
> The above assertion was being triggered because the paint rect was larger than the image itself.  This is because in the update function the paint rect is potentially extended to tile boundaries.
> 
> There was some offline discussion between Alok and me about whether we should have the tiler store explicit layer bounds and have different sized textures for outside tiles.  I thought this might be easy, but it ultimately added a lot more complication to the tiler's drawing and recycling mechanisms, where the tiler previously had a single tile size for all textures.
> 
> After some thought, I think the best solution is to just clip any request for uploading pixels from outside the image bounds to the image bounds itself.  Other layers have no problem painting outside their bounds, so this is really just an image-specific restriction.  Intersecting with the image bounds mimics the same behavior as before this patch, but the intersection has just moved to the updater from the tiler as a result of this refactoring.
> 

Adrienne: Thanks for the patch. Your solution would certainly work. I think the solution we discussed (keeping layer bounds in tiler) would also work while keeping the sizes for boundary tiles same. Do not clamp the tile size but rather source and destination rect in exactly the same way in your patch. Just move the clamping code to the tiler rather than ImageLayerTextureUpdater. Am I missing something?
Comment 25 Adrienne Walker 2011-05-18 08:07:15 PDT
(In reply to comment #24)

> Adrienne: Thanks for the patch. Your solution would certainly work. I think the solution we discussed (keeping layer bounds in tiler) would also work while keeping the sizes for boundary tiles same. Do not clamp the tile size but rather source and destination rect in exactly the same way in your patch. Just move the clamping code to the tiler rather than ImageLayerTextureUpdater. Am I missing something?

I considered that option as well, but thought this was better for two reasons.  Mostly, clamping in the image layer updater seemed simpler--there's no need to add code to ContentLayerChromium, ImageLayerChromium, LayerRendererChromium, and a number of places in WebViewImpl to keep track of the layer size.  Also, if we've gone ahead and overrendered and the paint rect fills the tile, why not upload all of those pixels in case we can use them later? This seems like a different use case than the images where the image size is fixed.
Comment 26 Alok Priyadarshi 2011-05-18 09:16:00 PDT
(In reply to comment #25)
> (In reply to comment #24)
> 
> > Adrienne: Thanks for the patch. Your solution would certainly work. I think the solution we discussed (keeping layer bounds in tiler) would also work while keeping the sizes for boundary tiles same. Do not clamp the tile size but rather source and destination rect in exactly the same way in your patch. Just move the clamping code to the tiler rather than ImageLayerTextureUpdater. Am I missing something?
> 

I think it is best to land this patch and worry about setting layer bounds later. I am on vacation today. I will merge your patch with mine and try to land it tomorrow.

I have an idea to fix 60983 in a simple way. A couple of questions for my understanding:

> I considered that option as well, but thought this was better for two reasons.  Mostly, clamping in the image layer updater seemed simpler--there's no need to add code to ContentLayerChromium, ImageLayerChromium, LayerRendererChromium, and a number of places in WebViewImpl to keep track of the layer size. 

Clamping in the image layer updater is certainly simpler. But it does seem a little weird that the tiler needs to paint outside the layer bounds. All the places that you mentioned already does setLayerPosition(). May be it can simply be replaced with setLayerRect().

> Also, if we've gone ahead and overrendered and the paint rect fills the tile, why not upload all of those pixels in case we can use them later? This seems like a different use case than the images where the image size is fixed.

I did not understand your last motivation. Are content/root layers able to paint outside the layer bounds? Even if they could in which cases could the pixels outside the bounds be used?
Comment 27 Adrienne Walker 2011-05-18 14:12:58 PDT
(In reply to comment #26)

> > Also, if we've gone ahead and overrendered and the paint rect fills the tile, why not upload all of those pixels in case we can use them later? This seems like a different use case than the images where the image size is fixed.
> 
> I did not understand your last motivation. Are content/root layers able to paint outside the layer bounds? Even if they could in which cases could the pixels outside the bounds be used?

I talked with jamesr and it turns out that I had a misunderstanding about frame resizing.  Apparently the entire root layer needs to be invalidated when the frame size changes.  So, there is no motivation to render outside the layer bounds because it will never be able to be used.
Comment 28 Adrienne Walker 2011-05-18 15:26:28 PDT
Created attachment 93992 [details]
Patch
Comment 29 James Robinson 2011-05-18 15:29:33 PDT
Comment on attachment 93992 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=93992&action=review

If at first you don't succeed, try try again!

> Source/WebCore/ChangeLog:2
> +2011-05-18  Alok Priyadarshi  <alokp@chromium.org>
> +            Adrienne Walker  <enne@google.com>

nit: same line, should be "Alok Priyadarshi  <alokp@chromium.org> and Adrienne Walker  <enne@google.com>"

do you use your @google.com for webkit patches or @chromium.org?

> Source/WebKit/chromium/ChangeLog:2
> +2011-05-18  Alok Priyadarshi  <alokp@chromium.org>
> +            Adrienne Walker  <enne@google.com>

same nits here
Comment 30 WebKit Review Bot 2011-05-18 15:43:56 PDT
Comment on attachment 93992 [details]
Patch

Attachment 93992 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8702913
Comment 31 WebKit Review Bot 2011-05-18 15:44:15 PDT
Comment on attachment 93992 [details]
Patch

Attachment 93992 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/8702915
Comment 32 Adrienne Walker 2011-05-18 15:49:48 PDT
Created attachment 93993 [details]
Patch
Comment 33 Adrienne Walker 2011-05-18 15:50:47 PDT
Ack.  I applied that patch wrong and didn't get any of the new files.  This patch should pass the ews bots.
Comment 34 Adrienne Walker 2011-05-18 16:53:28 PDT
Committed r86805: <http://trac.webkit.org/changeset/86805>