Closed Bug 1135883 Opened 5 years ago Closed 5 years ago

Implement GetMaxTextureSize in the basic compositor

Categories

(Core :: Graphics: Layers, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox37 --- fixed
firefox38 --- fixed
firefox39 --- fixed

People

(Reporter: dvander, Assigned: dvander)

References

Details

Attachments

(3 files)

Attached patch patchSplinter Review
Some crashtests with huge images fail when apz is enabled and tiling is disabled. Looks like the BasicCompositor allows any texture size.
Attachment #8568198 - Flags: review?(matt.woodrow)
Attached patch follow-upSplinter Review
Small follow-up, check whether or not ComputeMinBuffer() succeeded in TextureClient.
Attachment #8568219 - Flags: review?(matt.woodrow)
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.
Attachment #8568198 - Flags: review?(matt.woodrow) → review+
Attachment #8568219 - Flags: review?(matt.woodrow) → review+
https://hg.mozilla.org/mozilla-central/rev/99af18fdfdfe
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Depends on: 1139019
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?
(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.
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]:
Attachment #8574936 - Flags: review+
Attachment #8574936 - Flags: approval-mozilla-beta?
Attachment #8574936 - Flags: approval-mozilla-aurora?
Attachment #8574936 - Flags: approval-mozilla-beta?
Attachment #8574936 - Flags: approval-mozilla-beta+
Attachment #8574936 - Flags: approval-mozilla-aurora?
Attachment #8574936 - Flags: approval-mozilla-aurora+
No longer depends on: 1139019
You need to log in before you can comment on or make changes to this bug.