Bug 75779 - [Texmap] Move surface management from TextureMapperNode to TextureMapper
Summary: [Texmap] Move surface management from TextureMapperNode to TextureMapper
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 75778
  Show dependency treegraph
 
Reported: 2012-01-07 09:42 PST by Noam Rosenthal
Modified: 2012-01-09 03:54 PST (History)
3 users (show)

See Also:


Attachments
Patch (15.45 KB, patch)
2012-01-07 17:25 PST, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch (15.45 KB, patch)
2012-01-07 20:10 PST, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch (15.36 KB, patch)
2012-01-07 20:11 PST, Noam Rosenthal
mrobinson: review+
mrobinson: commit-queue-
Details | Formatted Diff | Diff
Patch (16.66 KB, patch)
2012-01-08 16:05 PST, Noam Rosenthal
kenneth: review+
Details | Formatted Diff | Diff
Patch (59.26 KB, patch)
2012-01-09 02:42 PST, Noam Rosenthal
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Patch (16.67 KB, patch)
2012-01-09 02:49 PST, Noam Rosenthal
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Noam Rosenthal 2012-01-07 09:42:08 PST
By having the surface management in TextureMapper, we'd be able to allocate/free surface from within TextureMapper. This is necessary for filters, where several intermediate surface might be needed.
Comment 1 Noam Rosenthal 2012-01-07 17:25:27 PST
Created attachment 121562 [details]
Patch
Comment 2 Igor Trindade Oliveira 2012-01-07 19:07:01 PST
Comment on attachment 121562 [details]
Patch

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

> Source/WebCore/platform/graphics/texmap/TextureMapper.cpp:45
> +

if i understood correctly, if (!newTexture) will be true just in the first loop iteration, so we could move this if out of for loop.
Doing something like:

RefPtr<BitmapTexture> newTexture = m_texturePool[0];
int index = 0;
for (size_t i = 1 ....)

what do you think?
Comment 3 Noam Rosenthal 2012-01-07 19:30:12 PST
(In reply to comment #2)
> (From update of attachment 121562 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=121562&action=review
> 
> > Source/WebCore/platform/graphics/texmap/TextureMapper.cpp:45
> > +
> 
> if i understood correctly, if (!newTexture) will be true just in the first loop iteration, so we could move this if out of for loop.
> Doing something like:
> 
> RefPtr<BitmapTexture> newTexture = m_texturePool[0];
> int index = 0;
> for (size_t i = 1 ....)
> 
> what do you think?

Yes, sounds less yucky. Will revise :)
Comment 4 Noam Rosenthal 2012-01-07 20:10:15 PST
Created attachment 121564 [details]
Patch
Comment 5 Noam Rosenthal 2012-01-07 20:11:49 PST
Created attachment 121565 [details]
Patch
Comment 6 Kenneth Rohde Christiansen 2012-01-08 14:38:44 PST
Comment on attachment 121565 [details]
Patch

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

> Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:461
>  bool TextureMapperNode::paintReflection(const TextureMapperPaintOptions& options
>  
>      // The mask has to be adjusted to target coordinates.
>      if (maskTexture) {
> -        maskSurface = options.surfaceManager->getIntermediateSurface();
> +        maskSurface = options.textureMapper->acquireTextureFromPool(options.textureMapper->viewportSize());

btw, why doesn't the textureMapperNode have a ref to the textureMapper? instead of passing it thru the options?

> Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:28
> +
> +PassRefPtr<BitmapTexture> TextureMapper::acquireTextureFromPool(const IntSize& size)
> +{

Why make this part of the TextureMapper? And not a singleton TexturePool? Is it not singleton in nature?

> Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:51
> +        // We choose the smallest texture that is equal or larger than the required size.
> +        if ((textureSize.width() < size.width() || textureSize.height() < size.height())
> +            || (newTextureSize.width() >= size.width()
> +                && newTextureSize.height() >= size.height()
> +                && newTextureSize.width() * newTextureSize.height() <= textureSize.width() * textureSize.height())) {
> +            continue;
> +        }

Couldnt this be styled a bit nicer?
Comment 7 Martin Robinson 2012-01-08 14:42:11 PST
Comment on attachment 121565 [details]
Patch

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

Okay! Please fix the style nit before landing.

> Source/WebCore/platform/graphics/texmap/TextureMapperNode.h:136
> +        : m_parent(0), m_effectTarget(0), m_opacity(1.0), m_textureMapper(0) { }

This should probably be:
TextureMapperNode()
    : m_parent(0)
    , m_effectTarget(0)
    , m_opacity(1),
    , m_textureMapper(0)
{
}

Note the style guide suggests remove the .0 from floats when it isn't important for arithmetic.
Comment 8 Martin Robinson 2012-01-08 14:44:29 PST
(In reply to comment #6)
> (From update of attachment 121565 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=121565&action=review

Whoops, sorry Noam. Kenneth left these comments while I was doing my review. I'm also interested in the answers to his questions, so maybe you might wait untli a nod from him to land this?
Comment 9 Noam Rosenthal 2012-01-08 15:32:53 PST
(In reply to comment #6)
> btw, why doesn't the textureMapperNode have a ref to the textureMapper? instead of passing it thru the options?
Because TextureMappers might be changed... they're tied to the painting context. You shouldn't really use TexftureMapper, except for during paint.

> 
> > Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:28
> > +
> > +PassRefPtr<BitmapTexture> TextureMapper::acquireTextureFromPool(const IntSize& size)
> > +{
> 
> Why make this part of the TextureMapper? And not a singleton TexturePool? Is it not singleton in nature?
No, it has to be cleaned up together with other TextureMapper-bound resources.

> 
> > Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:51
> > +        // We choose the smallest texture that is equal or larger than the required size.
> > +        if ((textureSize.width() < size.width() || textureSize.height() < size.height())
> > +            || (newTextureSize.width() >= size.width()
> > +                && newTextureSize.height() >= size.height()
> > +                && newTextureSize.width() * newTextureSize.height() <= textureSize.width() * textureSize.height())) {
> > +            continue;
> > +        }
> 
> Couldnt this be styled a bit nicer?
Will do.
Comment 10 Noam Rosenthal 2012-01-08 16:05:34 PST
Created attachment 121599 [details]
Patch

Addressed some of the comments with the patch; Other comments I addressed in previous reply.
Comment 11 Kenneth Rohde Christiansen 2012-01-09 01:30:04 PST
Comment on attachment 121599 [details]
Patch

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

> Source/WebCore/platform/graphics/texmap/TextureMapper.cpp:53
> +        bool selectedTextureContainsSize =

FitsSize?
Comment 12 Noam Rosenthal 2012-01-09 02:42:18 PST
Created attachment 121636 [details]
Patch
Comment 13 WebKit Review Bot 2012-01-09 02:44:14 PST
Comment on attachment 121636 [details]
Patch

Rejecting attachment 121636 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
apply', u'--force']" exit_code: 9

Error: the Git diff contains a binary file without the binary data in line: "Binary files /dev/null and b/LayoutTests/platform/qt/css3/filters/add-filter-rendering-expected.png differ".  Be sure to use the --binary flag when invoking "git diff" with diffs containing binary files. at /mnt/git/webkit-commit-queue/Tools/Scripts/VCSUtils.pm line 676, <ARGV> line 58.

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force']" exit_code: 9

Full output: http://queues.webkit.org/results/11183429
Comment 14 Noam Rosenthal 2012-01-09 02:49:13 PST
Created attachment 121637 [details]
Patch
Comment 15 WebKit Review Bot 2012-01-09 03:54:21 PST
Comment on attachment 121637 [details]
Patch

Clearing flags on attachment: 121637

Committed r104447: <http://trac.webkit.org/changeset/104447>
Comment 16 WebKit Review Bot 2012-01-09 03:54:26 PST
All reviewed patches have been landed.  Closing bug.