Bug 142386 - [TexMap] Seperate BitmapTexture related classes implementations from TextureMapper
Summary: [TexMap] Seperate BitmapTexture related classes implementations from TextureM...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Gwang Yoon Hwang
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-03-05 23:59 PST by Gwang Yoon Hwang
Modified: 2015-03-31 07:33 PDT (History)
5 users (show)

See Also:


Attachments
Patch (83.34 KB, patch)
2015-03-06 00:00 PST, Gwang Yoon Hwang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gwang Yoon Hwang 2015-03-05 23:59:18 PST
[TexMap] Seperate BitmapTexture related classes implementations from TextureMapper
Comment 1 Gwang Yoon Hwang 2015-03-06 00:00:33 PST
Created attachment 248048 [details]
Patch
Comment 2 Zan Dobersek 2015-03-27 02:25:36 PDT
Comment on attachment 248048 [details]
Patch

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

Looks nice, the position of these classes always bothered me.

There's plenty of space for further cleanups, but this is a great start.

> Source/WebCore/platform/graphics/texmap/BitmapTexture.cpp:39
> +    std::unique_ptr<ImageBuffer> imageBuffer = ImageBuffer::create(targetRect.size());
> +    GraphicsContext* context = imageBuffer->context();

Unrelated to the patch, but this would desperately need some improvement. It's creating a new ImageBuffer and the related cairo surface, allocating the necessary memory each time it's called.
Comment 3 Gwang Yoon Hwang 2015-03-27 19:27:09 PDT
(In reply to comment #2)
> Comment on attachment 248048 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=248048&action=review
> 
> Looks nice, the position of these classes always bothered me.
> 
> There's plenty of space for further cleanups, but this is a great start.
>

Not only clean ups, but also lots of improvement can be done after all. :)
 
> > Source/WebCore/platform/graphics/texmap/BitmapTexture.cpp:39
> > +    std::unique_ptr<ImageBuffer> imageBuffer = ImageBuffer::create(targetRect.size());
> > +    GraphicsContext* context = imageBuffer->context();
> 
> Unrelated to the patch, but this would desperately need some improvement.
> It's creating a new ImageBuffer and the related cairo surface, allocating
> the necessary memory each time it's called.

I agree, but in most of cases this method would not be called. I think in
Coordinated Graphics, we uses UpdateAtlas so we reuses ImageBuffer and Cairo surfaces,
but I need to check.

I think we need to remove TextureMapperImageBuffer and
remove all non-accelerated compositing code paths to simplify code paths.
Comment 4 WebKit Commit Bot 2015-03-27 20:15:52 PDT
Comment on attachment 248048 [details]
Patch

Clearing flags on attachment: 248048

Committed r182101: <http://trac.webkit.org/changeset/182101>
Comment 5 WebKit Commit Bot 2015-03-27 20:15:58 PDT
All reviewed patches have been landed.  Closing bug.
Comment 6 Csaba Osztrogonác 2015-03-27 22:35:01 PDT
(In reply to comment #4)
> Comment on attachment 248048 [details]
> Patch
> 
> Clearing flags on attachment: 248048
> 
> Committed r182101: <http://trac.webkit.org/changeset/182101>

It made compositing tests crash on the EFL bot.
Comment 7 Gwang Yoon Hwang 2015-03-27 23:01:20 PDT
(In reply to comment #6)
> (In reply to comment #4)
> > Comment on attachment 248048 [details]
> > Patch
> > 
> > Clearing flags on attachment: 248048
> > 
> > Committed r182101: <http://trac.webkit.org/changeset/182101>
> 
> It made compositing tests crash on the EFL bot.

Ossy, could you give me a more log about the crash?
I couldn't find any additional failure on EFL bot after this patch.
Comment 8 Csaba Osztrogonác 2015-03-28 02:28:14 PDT
before:
- https://build.webkit.org/builders/EFL%20Linux%2064-bit%20Release%20WK2/builds/20777
- 46 fail, only 1 crash from it

after:
- https://build.webkit.org/builders/EFL%20Linux%2064-bit%20Release%20WK2/builds/20778
- 64 fails, 18 compositing test crashes from it
Comment 9 Gwang Yoon Hwang 2015-03-29 23:10:49 PDT
(In reply to comment #8)
> before:
> -
> https://build.webkit.org/builders/EFL%20Linux%2064-bit%20Release%20WK2/
> builds/20777
> - 46 fail, only 1 crash from it
> 
> after:
> -
> https://build.webkit.org/builders/EFL%20Linux%2064-bit%20Release%20WK2/
> builds/20778
> - 64 fails, 18 compositing test crashes from it

Thanks for let me know.
I didn't noticed number changes. :/

But the problem is I cannot run WebKitEFL either using vm's driver or llvmpipe.
Do you know any of magic variable to run WebKitEFL?
I cannot enter to the opengl_x11 mode in EFL.
Comment 10 Csaba Osztrogonác 2015-03-30 03:09:53 PDT
(In reply to comment #9)
> (In reply to comment #8)
> > before:
> > -
> > https://build.webkit.org/builders/EFL%20Linux%2064-bit%20Release%20WK2/
> > builds/20777
> > - 46 fail, only 1 crash from it
> > 
> > after:
> > -
> > https://build.webkit.org/builders/EFL%20Linux%2064-bit%20Release%20WK2/
> > builds/20778
> > - 64 fails, 18 compositing test crashes from it
> 
> Thanks for let me know.
> I didn't noticed number changes. :/
> 
> But the problem is I cannot run WebKitEFL either using vm's driver or
> llvmpipe.
> Do you know any of magic variable to run WebKitEFL?
> I cannot enter to the opengl_x11 mode in EFL.

Let's continue in bug143214. Unfortunately I don't have any 
time today, but I'll try to check it in details tomorrow.