Closed
Bug 823610
Opened 8 years ago
Closed 8 years ago
Contact image only intermittently loads into crop view of pick activity
Categories
(Firefox OS Graveyard :: General, defect, P3)
Tracking
(blocking-basecamp:+, firefox19 fixed, firefox20 fixed, b2g18+ fixed)
People
(Reporter: cjones, Assigned: cjones)
References
Details
Attachments
(1 file, 1 obsolete file)
8.79 KB,
patch
|
justin.lebar+bug
:
review+
jgilbert
:
review+
|
Details | Diff | Splinter Review |
STR (1) Follow test at https://wiki.mozilla.org/B2G/Memory_acceptance_criteria#MW2:_Active_call_stays_active When loading the crop view, I sometimes see the image not load. I don't see any errors in logcat, though this test is explicitly designed to create memory pressure so that might be a factor. bb? to get on radar, not suggesting we block.
Updated•8 years ago
|
blocking-basecamp: ? → -
tracking-b2g18:
--- → +
Assignee | ||
Comment 1•8 years ago
|
||
I'm 99.9% sure now that this is caused by the gallery picker's WebGL context being killed off by a memory-pressure event. (1) Do we really want to do this to foreground apps? (2) What's the minimal patch to fix the gallery?
Assignee | ||
Comment 2•8 years ago
|
||
I'm not so sure about comment 1 anymore. However, in the last several days, I have seen this work exactly *once* in maybe 30 tries. I don't believe that people testing this (if any) have been testing in realistic conditions. This is a broken feature that we should either fix or cut.
blocking-basecamp: - → ?
Summary: Contact image intermittently fails to load into crop view of pick activity → Contact image only intermittently loads into crop view of pick activity
Assignee | ||
Comment 3•8 years ago
|
||
Note, many of those 30 tries have been outside the memory tests, just in "normal" usage.
Comment 5•8 years ago
|
||
Triage:BB+, P3, C3 - severe usability issue. please ignore comment 4
blocking-basecamp: - → +
Priority: -- → P3
Target Milestone: --- → B2G C4 (2jan on)
Assignee | ||
Comment 6•8 years ago
|
||
Did some testing I think comment 1 is right. I see a memory-pressure event fired in the gallery right when this happens.
Assignee | ||
Comment 7•8 years ago
|
||
Confirmed.
Assignee | ||
Comment 8•8 years ago
|
||
That's weird. I started adding contextlost handling to [1], but it wasn't trivial. But [2] implied it would be! ;) It doesn't really make sense to me to fire contextlost on visible canvases. That's pretty hard to determine in general, but we definitely know that canvases in background windows aren't visible. [1] https://github.com/mozilla-b2g/gaia/blob/master/apps/gallery/js/ImageEditor.js#L612 [2] http://www.khronos.org/webgl/wiki/HandlingContextLost
Assignee | ||
Comment 9•8 years ago
|
||
In the style of bug 818766. GL hunks -> jgilbert, dom/ipc -> jlebar.
Assignee: nobody → jones.chris.g
Attachment #696852 -
Flags: review?(justin.lebar+bug)
Attachment #696852 -
Flags: review?(jgilbert)
Attachment #696852 -
Flags: feedback?(bjacob)
Comment 10•8 years ago
|
||
Comment on attachment 696852 [details] [diff] [review] Make discarding GL contexts of foreground pages on memory pressure pref-able, and pref off for b2g Review of attachment 696852 [details] [diff] [review]: ----------------------------------------------------------------- WebGL and pref parts look good.
Attachment #696852 -
Flags: feedback?(bjacob) → feedback+
Comment 11•8 years ago
|
||
Comment on attachment 696852 [details] [diff] [review] Make discarding GL contexts of foreground pages on memory pressure pref-able, and pref off for b2g Review of attachment 696852 [details] [diff] [review]: ----------------------------------------------------------------- Looks fine, though the name for the var could be improved. To that end, the pref should probably be kept matching if the var changes. ::: content/canvas/src/WebGLContext.cpp @@ +67,5 @@ > bool wantToLoseContext = true; > > + if (!mContext->mLoseContextInForeground && IsCurrentProcessForeground()) > + wantToLoseContext = false; > + else if (!nsCRT::strcmp(aSomeData, I still dislike `!strcmp()` as a meme, but this code was already here, so don't worry about it. @@ +183,5 @@ > mContextRestorer = do_CreateInstance("@mozilla.org/timer;1"); > mContextStatus = ContextStable; > mContextLostErrorSet = false; > mLoseContextOnHeapMinimize = false; > + mLoseContextInForeground = true; `mCanLoseContextInForeground` is probably better. `mLoseContextInForeground` sounds like a really silly thing to do.
Attachment #696852 -
Flags: review?(jgilbert) → review+
Comment 12•8 years ago
|
||
Does not seems a Gaia bug anymore. Let's switch the component to reflect that.
Component: Gaia::Gallery → General
QA Contact: jhammink
Assignee | ||
Comment 13•8 years ago
|
||
Updated names.
Attachment #696852 -
Attachment is obsolete: true
Attachment #696852 -
Flags: review?(justin.lebar+bug)
Attachment #697289 -
Flags: review?
Assignee | ||
Comment 14•8 years ago
|
||
Comment on attachment 697289 [details] [diff] [review] Make discarding GL contexts of foreground pages on memory pressure pref-able, and pref off for b2g , v1.1 Let's try again ...
Attachment #697289 -
Flags: review?(justin.lebar+bug)
Attachment #697289 -
Flags: review?(jgilbert)
Attachment #697289 -
Flags: review?
Comment 15•8 years ago
|
||
Comment on attachment 697289 [details] [diff] [review] Make discarding GL contexts of foreground pages on memory pressure pref-able, and pref off for b2g , v1.1 Review of attachment 697289 [details] [diff] [review]: ----------------------------------------------------------------- Cool, thanks.
Attachment #697289 -
Flags: review?(jgilbert) → review+
Comment 16•8 years ago
|
||
Comment on attachment 697289 [details] [diff] [review] Make discarding GL contexts of foreground pages on memory pressure pref-able, and pref off for b2g , v1.1 >diff --git a/content/canvas/src/WebGLContextValidate.cpp b/content/canvas/src/WebGLContextValidate.cpp >index be6c13e..5052460 100644 >--- a/content/canvas/src/WebGLContextValidate.cpp >+++ b/content/canvas/src/WebGLContextValidate.cpp >@@ -700,16 +700,17 @@ WebGLContext::InitAndValidateGL() > if (error != LOCAL_GL_NO_ERROR) { > GenerateWarning("GL error 0x%x occurred during OpenGL context initialization, before WebGL initialization!", error); > return false; > } > > mMinCapability = Preferences::GetBool("webgl.min_capability_mode", false); > mDisableExtensions = Preferences::GetBool("webgl.disable-extensions", false); > mLoseContextOnHeapMinimize = Preferences::GetBool("webgl.lose-context-on-heap-minimize", false); >+ mCanLoseContextInForeground = Preferences::GetBool("webgl.can-lose-context-in-foreground", true); The top of all.js says > * SYNTAX HINTS: > * > * - Dashes are delimiters; use underscores instead. Is that wrong? (Sure seems like it.) >diff --git a/dom/ipc/ProcessPriorityManager.h b/dom/ipc/ProcessPriorityManager.h >index c320611..f149a30 100644 >--- a/dom/ipc/ProcessPriorityManager.h >+++ b/dom/ipc/ProcessPriorityManager.h >@@ -21,13 +21,20 @@ namespace ipc { > * > * Hal may adjust this process's operating system priority (e.g. niceness, on > * *nix) according to these notificaitons. > * > * This function call does nothing if the pref for OOP tabs is not set. > */ > void InitProcessPriorityManager(); > >+/** >+ * True iff the current process has foreground or higher priority as >+ * computed by DOM visibility. The returned answer may not match the >+ * actual OS process priority, for short intervals. >+ */ >+bool IsCurrentProcessForeground(); Would you mind "CurrentProcessIsForeground"? >diff --git a/dom/ipc/ProcessPriorityManager.cpp b/dom/ipc/ProcessPriorityManager.cpp >index 749dea5..e8a66b8 100644 >--- a/dom/ipc/ProcessPriorityManager.cpp >+++ b/dom/ipc/ProcessPriorityManager.cpp >@@ -378,15 +384,22 @@ InitProcessPriorityManager() > // backgrounded). > if (XRE_GetProcessType() == GeckoProcessType_Default) { > LOG("This is the master process."); > hal::SetProcessPriority(getpid(), PROCESS_PRIORITY_MASTER); > return; > } > > // This object is held alive by the observer service. >- nsRefPtr<ProcessPriorityManager> mgr = new ProcessPriorityManager(); >- mgr->Init(); >+ sManager = new ProcessPriorityManager(); >+ sManager->Init(); >+ ClearOnShutdown(&sManager); This comment is no longer right. >+bool >+IsCurrentProcessForeground() >+{ >+ return sManager->GetPriority() >= PROCESS_PRIORITY_FOREGROUND; > } Can you add a comment on the enum definition that anything greater than or equal to FOREGROUND is considered "foreground"? Otherwise I don't mind explicitly listing the possibilities here, since there are only two. :)
Attachment #697289 -
Flags: review?(justin.lebar+bug) → review+
Assignee | ||
Comment 17•8 years ago
|
||
(In reply to Justin Lebar [:jlebar] from comment #16) > Comment on attachment 697289 [details] [diff] [review] > The top of all.js says > > > * SYNTAX HINTS: > > * > > * - Dashes are delimiters; use underscores instead. > > Is that wrong? (Sure seems like it.) No clue. I'm just blindly following existing webgl pref style. > > >diff --git a/dom/ipc/ProcessPriorityManager.h b/dom/ipc/ProcessPriorityManager.h > >index c320611..f149a30 100644 > >--- a/dom/ipc/ProcessPriorityManager.h > >+++ b/dom/ipc/ProcessPriorityManager.h > > >@@ -21,13 +21,20 @@ namespace ipc { > > * > > * Hal may adjust this process's operating system priority (e.g. niceness, on > > * *nix) according to these notificaitons. > > * > > * This function call does nothing if the pref for OOP tabs is not set. > > */ > > void InitProcessPriorityManager(); > > > >+/** > >+ * True iff the current process has foreground or higher priority as > >+ * computed by DOM visibility. The returned answer may not match the > >+ * actual OS process priority, for short intervals. > >+ */ > >+bool IsCurrentProcessForeground(); > > Would you mind "CurrentProcessIsForeground"? > Fine by me. > >diff --git a/dom/ipc/ProcessPriorityManager.cpp b/dom/ipc/ProcessPriorityManager.cpp > >index 749dea5..e8a66b8 100644 > >--- a/dom/ipc/ProcessPriorityManager.cpp > >+++ b/dom/ipc/ProcessPriorityManager.cpp > > >@@ -378,15 +384,22 @@ InitProcessPriorityManager() > > // backgrounded). > > if (XRE_GetProcessType() == GeckoProcessType_Default) { > > LOG("This is the master process."); > > hal::SetProcessPriority(getpid(), PROCESS_PRIORITY_MASTER); > > return; > > } > > > > // This object is held alive by the observer service. > >- nsRefPtr<ProcessPriorityManager> mgr = new ProcessPriorityManager(); > >- mgr->Init(); > >+ sManager = new ProcessPriorityManager(); > >+ sManager->Init(); > >+ ClearOnShutdown(&sManager); > > This comment is no longer right. > Removed. > >+bool > >+IsCurrentProcessForeground() > >+{ > >+ return sManager->GetPriority() >= PROCESS_PRIORITY_FOREGROUND; > > } > > Can you add a comment on the enum definition that anything greater than or > equal to FOREGROUND is considered "foreground"? > Done.
Assignee | ||
Comment 18•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc3fd7c51000
Comment 19•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/fc3fd7c51000
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Comment 20•8 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/1aae9f7f0cf5 https://hg.mozilla.org/releases/mozilla-b2g18/rev/0a21cf9f0047
You need to log in
before you can comment on or make changes to this bug.
Description
•