David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Description • 9 years ago

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

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 1 • 9 years ago

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

	Matt Woodrow (:mattwoodrow)
	Comment 2 • 9 years ago

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.

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 3 • 9 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 4 • 9 years ago

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

	Milan Sreckovic [:milan] (needinfo for best results)
	Comment 5 • 9 years ago

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?

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 6 • 9 years ago

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

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 7 • 9 years ago

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]:

	Ryan VanderMeulen [:RyanVM]
	Comment 8 • 9 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 9 • 9 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 10 • 9 years ago

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

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 11 • 9 years ago

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