Closed Bug 1060790 Opened 9 years ago Closed 9 years ago

Call glFlush() to SkiaGL's GLContext on memory-pressure

Categories

(Core :: Graphics, defect)

Product:
Core
Component:
Graphics
Platform:
ARM
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla35
Project Flags:
blocking-b2g 2.0+
Tracking Flags:
Tracking Status
firefox33 --- wontfix
firefox34 --- fixed
firefox35 --- fixed
b2g-v2.0 --- fixed
b2g-v2.0M --- fixed
b2g-v2.1 --- fixed
b2g-v2.2 --- fixed

People

(Reporter: sotaro, Assigned: sotaro)

References

Details

(Whiteboard: [MemShrink:P2])

Attachments

(1 file, 1 obsolete file)

patch - Call glFlush() to SkiaGL's GLContext on memory-pressure r=gw280
818 bytes, patch
sotaro
: review+
bajaj
: approval-mozilla-aurora+
bajaj
: approval-mozilla-b2g32+
Details | Diff | Splinter Review 
+++ This bug was initially created as a clone of Bug #1049195 +++

During investigating Bug 1049195, I recognized that glFush()/glFinish() works to reduce kgls memory usage. But on memory-pressure, glFlush() is not called to SkiaGL's GLContext.
Blocks: 1049251
Attachment #8481780 - Flags: review?(gwright)
[Blocking Requested - why for this release]: Blocking 2.0+ bugs.
blocking-b2g: --- → 2.0?
Comment on attachment 8481780 [details] [diff] [review]
patch - Call glFlush() to SkiaGL's GLContext on memory-pressure

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

Why not call GrContext::flush()?
Whiteboard: [MemShrink]
blocking-b2g: 2.0? → 2.0+
(In reply to George Wright (:gw280 PTO Aug 15th - 24th) from comment #3)
> Comment on attachment 8481780 [details] [diff] [review]
> patch - Call glFlush() to SkiaGL's GLContext on memory-pressure
> 
> Review of attachment 8481780 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Why not call GrContext::flush()?

GrContext::flush() is called in GrContext::freeGpuResources(). But GrContext::flush() does not call glFlush().
http://mxr.mozilla.org/mozilla-central/source/gfx/skia/trunk/src/gpu/GrContext.cpp#216
(In reply to Sotaro Ikeda [:sotaro] from comment #5)
> GrContext::flush() calls GrInOrderDrawBuffer::flush().
> http://mxr.mozilla.org/mozilla-central/source/gfx/skia/trunk/src/gpu/
> GrInOrderDrawBuffer.cpp#556

GrInOrderDrawBuffer::flush() does not call glFlush().
Comment on attachment 8481780 [details] [diff] [review]
patch - Call glFlush() to SkiaGL's GLContext on memory-pressure

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

Looks fine then, but please add a comment about the flush call to say that GrContext::flush() doesn't call glFlush.
Attachment #8481780 - Flags: review?(gwright) → review+
Assignee: nobody → sotaro.ikeda.g
Apply the comment.
Attachment #8481780 - Attachment is obsolete: true
Attachment #8482944 - Flags: review+
Whiteboard: [MemShrink] → [MemShrink:P2]
https://hg.mozilla.org/mozilla-central/rev/29b4ff415e17
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Note that due to recent policy changes, all B2G uplifts needs approval now regardless of blocking status. Please request aurora and b2g32 approval on this patch when you get a chance. Sorry for the inconvenience.
status-b2g-v2.0: --- → affected
status-b2g-v2.1: --- → affected
status-b2g-v2.2: --- → fixed
status-firefox33: --- → wontfix
status-firefox34: --- → affected
status-firefox35: --- → fixed
Flags: needinfo?(sotaro.ikeda.g)
Comment on attachment 8482944 [details] [diff] [review]
patch - Call glFlush() to SkiaGL's GLContext on memory-pressure r=gw280

NOTE: This flag is now for security issues only. Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): None.
User impact if declined: An application might be killed more than the fix is applied.
Testing completed: Tested locally on flame device.
Risk to taking this patch (and alternatives if risky): Low.
String or UUID changes made by this patch:
Attachment #8482944 - Flags: approval-mozilla-b2g32?
Attachment #8482944 - Flags: approval-mozilla-aurora?
Flags: needinfo?(sotaro.ikeda.g)
(In reply to Sotaro Ikeda [:sotaro] from comment #13)
> 
> [Approval Request Comment]
> Bug caused by (feature/regressing bug #): None.
> User impact if declined: An application might be killed more than the fix is
> applied.

An application might be killed more than the fix is applied because of low memory.
Attachment #8482944 - Flags: approval-mozilla-b2g32?
Attachment #8482944 - Flags: approval-mozilla-b2g32+
Attachment #8482944 - Flags: approval-mozilla-aurora?
Attachment #8482944 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.