Bug 75779 – [Texmap] Move surface management from TextureMapperNode to TextureMapper

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Noam Rosenthal]

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.

[Noam Rosenthal]

Created attachment 121562 [details]
Patch

[Igor Trindade Oliveira]

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?

[Noam Rosenthal]

(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 :)

[Noam Rosenthal]

Created attachment 121564 [details]
Patch

[Noam Rosenthal]

Created attachment 121565 [details]
Patch

[Kenneth Rohde Christiansen]

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?

[Martin Robinson]

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.

[Martin Robinson]

(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?

[Noam Rosenthal]

(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.

[Noam Rosenthal]

Created attachment 121599 [details]
Patch

Addressed some of the comments with the patch; Other comments I addressed in previous reply.

[Kenneth Rohde Christiansen]

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?

[Noam Rosenthal]

Created attachment 121636 [details]
Patch

[WebKit Review Bot]

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

[Noam Rosenthal]

Created attachment 121637 [details]
Patch

[WebKit Review Bot]

Comment on attachment 121637 [details]
Patch

Clearing flags on attachment: 121637

Committed r104447: <http://trac.webkit.org/changeset/104447>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.