Bug 84822 - [chromium] REGRESSION(112286) Compositor initialization blocks for program compilation / linking
Summary: [chromium] REGRESSION(112286) Compositor initialization blocks for program co...
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: James Robinson
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-04-24 20:09 PDT by James Robinson
Modified: 2012-04-25 21:49 PDT (History)
3 users (show)

See Also:


Attachments
Patch (8.48 KB, patch)
2012-04-24 20:25 PDT, James Robinson
no flags Details | Formatted Diff | Diff
Patch (9.29 KB, patch)
2012-04-24 23:48 PDT, James Robinson
no flags Details | Formatted Diff | Diff
Patch for landing (10.21 KB, patch)
2012-04-25 21:11 PDT, James Robinson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description James Robinson 2012-04-24 20:09:09 PDT
[chromium] REGRESSION(112286) Compositor initialization blocks for program compilation / linking
Comment 1 James Robinson 2012-04-24 20:25:49 PDT
Created attachment 138727 [details]
Patch
Comment 2 James Robinson 2012-04-24 23:48:46 PDT
Created attachment 138744 [details]
Patch
Comment 3 Dana Jansens 2012-04-25 09:34:04 PDT
Comment on attachment 138744 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/TextureCopier.cpp:82
> +    if (m_blitProgram->initialized())

is that supposed to be if (!initialized) ?
Comment 4 James Robinson 2012-04-25 10:52:18 PDT
Oh, yes (interesting that no layout tests break)
Comment 5 Adrienne Walker 2012-04-25 18:03:52 PDT
(In reply to comment #4)
> Oh, yes (interesting that no layout tests break)

It'd just re-get the uniforms on ever ycopy, so it's inefficient but still correct.  Can I trouble you to add an assert to ProgramBinding::initialize() to make sure we don't do this?
Comment 6 Adrienne Walker 2012-04-25 18:05:31 PDT
Comment on attachment 138744 [details]
Patch

Other than than the initialized() check, looks great.  Thanks for writing a test.  :)
Comment 7 James Robinson 2012-04-25 21:10:24 PDT
(In reply to comment #5)
> (In reply to comment #4)
> > Oh, yes (interesting that no layout tests break)
> 
> It'd just re-get the uniforms on ever ycopy, so it's inefficient but still correct.  Can I trouble you to add an assert to ProgramBinding::initialize() to make sure we don't do this?

No, since i'm not calling initialize() unless initialized() is true, I think it will never get the uniforms at all.  The only uniform on the program is the s_texture on the fragment shader.  So I have no idea WTF it was doing.

Anyway, added ASSERT()s for both ways - calling initialize() too much or calling program() before initialize() and it seems good now.
Comment 8 James Robinson 2012-04-25 21:11:40 PDT
Created attachment 138928 [details]
Patch for landing
Comment 9 WebKit Review Bot 2012-04-25 21:49:42 PDT
Comment on attachment 138928 [details]
Patch for landing

Clearing flags on attachment: 138928

Committed r115285: <http://trac.webkit.org/changeset/115285>
Comment 10 WebKit Review Bot 2012-04-25 21:49:47 PDT
All reviewed patches have been landed.  Closing bug.