Bug 163439 – [Cairo] REGRESSION(r206481): GraphicsContext3D::ImageExtractor fails to extract images

• Status: REOPENED
• Component: New Bugs

[Zan Dobersek]

[Cairo] GraphicsContext3D::ImageExtractor fails to extract images

[Zan Dobersek]

Created attachment 291597 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 291597 [details]
Patch

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

> Source/WebCore/ChangeLog:14
> +        In the Cairo implementation of GraphicsContext3D::ImageExtractor,
> +        don't check for frame completeness at index 0. This information
> +        is now cached only after the frame for that index is decoded and
> +        marked as completed, which is done after this check.
> +
> +        Becuase of this the current check forces extractImage() to return
> +        early and abort WebGL texture uploads from image sources.

If this a recent regression after the image decoder refactorings? it would be great to mention a revision in the commit message, so that I'll know in the future that I don't need this in any stable branch.

[Zan Dobersek]

It was probably introduced in r206481.
https://trac.webkit.org/changeset/206481

[Zan Dobersek]

Created attachment 291600 [details]
Patch for landing

[Zan Dobersek]

Comment on attachment 291600 [details]
Patch for landing

Clearing flags on attachment: 291600

Committed r207332: <http://trac.webkit.org/changeset/207332>

[Zan Dobersek]

All reviewed patches have been landed.  Closing bug.

[Said Abou-Hallawa]

(In reply to comment #3)
> It was probably introduced in r206481.
> https://trac.webkit.org/changeset/206481

Yes this is true. In this change I made frameIsCompleteAtIndex() reads whatever in the cache without having to ask the decoder about the completeness of the frame and without trying to cache a new frame.

The reason for this change is this information is mainly useful to the ImageFrameCache. It can be sued to decide whether we can use the current frame or we need to try recaching a new one. For other places of the code like GraphicsContext3D::ImageExtractor, it should not matter whether the frame is complete or not. The completeness of a frame represents whether the frame image can fully or partially displayed. And we need to display it even if it is partially decoded.

[Said Abou-Hallawa]

Comment on attachment 291597 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        marked as completed, which is done after this check.

What are the repro steps of this bug? Can't we include a test case in this patch?

[Said Abou-Hallawa]

Comment on attachment 291597 [details]
Patch

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

> Source/WebCore/platform/graphics/cairo/GraphicsContext3DCairo.cpp:226
> -        if (!decoder.frameCount() || !decoder.frameIsCompleteAtIndex(0))
> +        if (!decoder.frameCount())

The function GraphicsContext3D::ImageExtractor::extractImage() in WebCore/platform/graphics/cg/GraphicsContext3D.cpp does not have the check decoder.frameIsCompleteAtIndex(). 

But the function GraphicsContext3D::ImageExtractor::extractImage() in WebCore/platform/graphics/efl/GraphicsContext3D.cpp does have the check decoder.frameIsCompleteAtIndex(). 

        if (!decoder.frameCount() || !decoder.frameIsCompleteAtIndex(0))
            return false;

I think we need to apply the same fix there as well. Maybe we need to move some of the code of this function to the cross platform file: WebCore/platform/graphics/GraphicsContext3D.cpp to avoid repeating the code and avoid fixing the same bug in different places.

[Said Abou-Hallawa]

Reopening the bug to apply the same fix to the elf port and to add a test if possible.

[Zan Dobersek]

(In reply to comment #8)
> Comment on attachment 291597 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=291597&action=review
> 
> > Source/WebCore/ChangeLog:11
> > +        marked as completed, which is done after this check.
> 
> What are the repro steps of this bug? Can't we include a test case in this
> patch?

This is reproducible by running any WebGL example that does texture uploads from an image source (specifically image, canvas or video elements). There's plenty of tests for that, but the EFL and GTK+ ports probably didn't catch them because they either don't run those tests, or don't compare pixel results from these tests.

This problem should have stored a relevant error on the GraphicsContext3D object, but I suppose again the ports don't run a test which would propagate this problem into a test failure.