1135883 - Implement GetMaxTextureSize in the basic compositor

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: patch

Some crashtests with huge images fail when apz is enabled and tiling is disabled. Looks like the BasicCompositor allows any texture size.

Comment 1

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: follow-up

Small follow-up, check whether or not ComputeMinBuffer() succeeded in TextureClient.

Comment 2

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8568198 [details] [diff] [review]
patch

Review of attachment 8568198 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/2d/2D.h
@@ +1165,5 @@
>  
>    // This is a little hacky at the moment, but we want to have this data. Bug 1068613.
>    static void SetLogForwarder(LogForwarder* aLogFwd);
>  
> +  static uint32_t GetMaxTextureSize(BackendType aType);

Call this one GetMaxSurfaceSize as well.

Comment 3

[David Anderson [:dvander] - inactive, e-mail if emergency]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/99af18fdfdfe

Comment 4

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/99af18fdfdfe

Comment 5

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8568219 [details] [diff] [review]
follow-up

Review of attachment 8568219 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/client/TextureClient.cpp
@@ +744,5 @@
>      return false;
>    }
>  
>    uint32_t bufSize = ImageDataSerializer::ComputeMinBufferSize(aSize, mFormat);
> +  if (!bufSize || !Allocate(bufSize)) {

Why did we need this if Allocate "appears to" handle bufSize == 0 case?

Comment 6

[David Anderson [:dvander] - inactive, e-mail if emergency]

(In reply to Milan Sreckovic [:milan] from comment #5)
> Comment on attachment 8568219 [details] [diff] [review]
> follow-up
> 
> Review of attachment 8568219 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/client/TextureClient.cpp
> @@ +744,5 @@
> >      return false;
> >    }
> >  
> >    uint32_t bufSize = ImageDataSerializer::ComputeMinBufferSize(aSize, mFormat);
> > +  if (!bufSize || !Allocate(bufSize)) {
> 
> Why did we need this if Allocate "appears to" handle bufSize == 0 case?

Some of the implementations of Allocate() don't check for bufSize == 0, and some pass the allocation off into other functions that don't either. It looks like all the callers of ComputeMinBufferSize() check for 0 though.

Comment 7

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: rebased patch for beta/aurora

Approval Request Comment
[Feature/regressing bug #]: OMTC
[User impact if declined]: If hardware acceleration is disabled due to a driver backlist, the browser can crash trying to render huge images.
[Describe test coverage new/current, TreeHerder]: This test was uncovered by running reftests with e10s and the basic compositor.
[Risks and why]: None, this simply adds a missing check that an allocation would fail.
[String/UUID change made/needed]:

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6d66dccc94f8

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/096b8eb2590d

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Backed out for Werror bustage.
https://hg.mozilla.org/releases/mozilla-aurora/rev/115d3d727f45
https://hg.mozilla.org/releases/mozilla-beta/rev/b526678ba6d2

https://treeherder.mozilla.org/logviewer.html#?job_id=660476&repo=mozilla-aurora

Comment 11

[David Anderson [:dvander] - inactive, e-mail if emergency]

https://hg.mozilla.org/releases/mozilla-beta/rev/2d58dd0bfaf7
https://hg.mozilla.org/releases/mozilla-aurora/rev/b673d5f84d9f