Camera - make cycle collection participants

RESOLVED FIXED

Status

Firefox OS
General
--
major
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: mikeh, Assigned: mikeh)

Tracking

unspecified
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(blocking-basecamp:+)

Details

Attachments

(1 attachment, 20 obsolete attachments)

158.72 KB, patch
Details | Diff | Splinter Review
CameraControl and CameraCapabilities reference each other and need to be made cycle collection participants.
Summary: [b2g] Camera - make cycle collection participants → Camera - make cycle collection participants
blocking-basecamp: --- → ?
As long as we run the camera app out-of-process this will only leak two objects until the camera app is shut down. So don't think this is a blocker.
blocking-basecamp: ? → -
Created attachment 651419 [details] [diff] [review]
WIP - cycle collection refactoring of code
Created attachment 651422 [details] [diff] [review]
WIP - cycle collection refactoring of code
Attachment #651419 - Attachment is obsolete: true
Created attachment 651425 [details] [diff] [review]
WIP - cycle collection refactoring of code
Attachment #651422 - Attachment is obsolete: true
Created attachment 651456 [details] [diff] [review]
Part 1: DOM-facing cycle-collection participants
Attachment #651456 - Flags: review?(khuey)
Created attachment 651458 [details] [diff] [review]
Part 2: Gonk-facing core implementation
Attachment #651458 - Flags: review?
Created attachment 651469 [details] [diff] [review]
WIP - cycle collection refactoring of code
Attachment #651425 - Attachment is obsolete: true
Created attachment 651470 [details] [diff] [review]
Part 2: Gonk-facing core implementation
Attachment #651458 - Attachment is obsolete: true
Attachment #651458 - Flags: review?
Attachment #651470 - Flags: review?
Attachment #651470 - Flags: review? → review?(bent.mozilla)
review? ping.
Comment on attachment 651456 [details] [diff] [review]
Part 1: DOM-facing cycle-collection participants

I don't think I'm a very good reviewer for this.  Roc wrote the media stream code, maybe he can take a look?
Attachment #651456 - Flags: review?(roc)
Created attachment 653522 [details] [diff] [review]
WIP - cycle collection refactoring of code

Fix up some bit-rot.  This patch should apply cleanly.
Attachment #651469 - Attachment is obsolete: true
Comment on attachment 651456 [details] [diff] [review]
Part 1: DOM-facing cycle-collection participants

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

You should use hg copy/hg move so that your new files preserve history from the old ones (and the patch looks better).
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #12)
> Comment on attachment 651456 [details] [diff] [review]
> Part 1: DOM-facing cycle-collection participants
> 
> Review of attachment 651456 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> You should use hg copy/hg move so that your new files preserve history from
> the old ones (and the patch looks better).

I have the moves/copies/removes in git, but it all seems to get lost in the shuffle to a patch.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #14)
> OK, but we use hg here :-).

You should come visit b2g-land some day.  ;)
Status: NEW → ASSIGNED
Mike, why don't you just set your gecko dir in b2g to point to an hg repo, and then write hg gecko patches and git gaia patches?
Created attachment 653861 [details] [diff] [review]
WIP - cycle collection refactoring of code

Rebase properly this time.  /git-rage.
Attachment #653522 - Attachment is obsolete: true
Created attachment 656469 [details] [diff] [review]
WIP - cycle collection refactoring of code

Rebased to latest version of tree.
Attachment #653861 - Attachment is obsolete: true
Created attachment 656985 [details] [diff] [review]
WIP - cycle collection refactoring of code

This includes several changes intended to make the camera easier on resources:
- cycle collection participation of DOM-facing objects
- camera hardware lifetime extended to entire lifetime of camera app
- generational window buffer manager
- cleaner logging control: if you have a debug build, you can enable logging in the camera code by setting the NSPR_LOG_MODULES environment variable from 0 to 5, where 0 is nothing, 1 is errors only, 2 includes warnings, 3 includes informational prints, 4 includes tracing (i.e. entry and exit to functions, constructors, and destructors), and 5 includes camera object reference count tracing!

The last option generates a LOT (megabytes) of logcat output, but can be handy for tracking down memory leaks.

This patch is rebased to use only standard C/++ types; PR* types are gone.
Attachment #656469 - Attachment is obsolete: true
Created attachment 656987 [details]
WIP - camera.js required to use new API

The new WIP includes a new CameraControl API, resumePreview(), which is used to restart the preview stream after taking a picture.

This sample .js shows how to use it.

With these changes, the preview now immediately restarts after taking a photo.
Created attachment 657192 [details]
Memory usage after new Camera API

I tried to apply both of your patches. At first, it did not produced real improvement. Then I decided to only keep one picture preview in the film strip. Now, I've been able to take a lot of picture, with a quite steady memory usage. After a while, it freezed. A couple of minutes later (while under screenlock) it unfreezed.
Looks like it did freeze only because I tapped on the preview and it triggered autofocus. If I retap, it unfreezes. I'll post memory capture with new Camera API but no photoTaken trick.
Created attachment 657193 [details]
Memory usage after new Camera API and displaying film strip

Now, measuring the memory with the same patches, new Camera API and this time the film strip of photos taken. b2g process gets killed by kernel (oom) after 5 or 6 pictures, and memory usage is growing ...
Created attachment 657239 [details] [diff] [review]
WIP - cycle collection refactoring of code

Latest patch, includes bugfixen for camera shutdown leaks.
Attachment #656985 - Attachment is obsolete: true
(In reply to Alexandre LISSY from comment #22)
> Looks like it did freeze only because I tapped on the preview and it
> triggered autofocus. If I retap, it unfreezes. I'll post memory capture with
> new Camera API but no photoTaken trick.

Yes, I put that in there to test stream pausing.  (It's only meant for Otoro, which doesn't support auto-focusing.)
(In reply to Mike Habicher [:mikeh] from comment #24)
> Created attachment 657239 [details] [diff] [review]
> WIP - cycle collection refactoring of code
> 
> Latest patch, includes bugfixen for camera shutdown leaks.

Is it normal that your patch still references types like PRUint64 ?
Created attachment 657253 [details] [diff] [review]
WIP - cycle collection refactoring of code

A final bugfix to make sure we don't call abandon() on a non-existant window.
Attachment #657239 - Attachment is obsolete: true
Created attachment 657284 [details] [diff] [review]
WIP - cycle collection refactoring of code

Includes all previous changes, and fixes the non-debug build.
Attachment #657253 - Attachment is obsolete: true
(In reply to Alexandre LISSY from comment #26)
> (In reply to Mike Habicher [:mikeh] from comment #24)
> > Created attachment 657239 [details] [diff] [review]
> > WIP - cycle collection refactoring of code
> > 
> > Latest patch, includes bugfixen for camera shutdown leaks.
> 
> Is it normal that your patch still references types like PRUint64 ?

They're all gone now.
(In reply to Mike Habicher [:mikeh] from comment #29)
> (In reply to Alexandre LISSY from comment #26)
> > (In reply to Mike Habicher [:mikeh] from comment #24)
> > > Created attachment 657239 [details] [diff] [review]
> > > WIP - cycle collection refactoring of code
> > > 
> > > Latest patch, includes bugfixen for camera shutdown leaks.
> > 
> > Is it normal that your patch still references types like PRUint64 ?
> 
> They're all gone now.

Nope, see https://bugzilla.mozilla.org/attachment.cgi?id=657284&action=diff#a/dom/camera/DOMCameraManager.cpp_sec3, on one side you have "nsDOMCameraManager::nsDOMCameraManager(PRUint64 aWindowId)" and on the other "nsDOMCameraManager::nsDOMCameraManager(uint64_t aWindowId)" but current gecko already uses this prototype, see https://github.com/mozilla/mozilla-central/blob/master/dom/camera/DOMCameraManager.cpp#L33
Created attachment 657297 [details] [diff] [review]
Cycle collection refactoring of DOM-facing objects, clean-up

Final patch for review.  My apologies for the mucked up renames--if you know how to fix this, I'll apply it.  Otherwise, I can prepare some offline diffs manually.
Attachment #651456 - Attachment is obsolete: true
Attachment #651470 - Attachment is obsolete: true
Attachment #657284 - Attachment is obsolete: true
Attachment #651456 - Flags: review?(roc)
Attachment #651456 - Flags: review?(khuey)
Attachment #651470 - Flags: review?(bent.mozilla)
Attachment #657297 - Flags: review?(jst)
Created attachment 657359 [details] [diff] [review]
Cycle collection refactoring of DOM-facing objects, clean-up

Rebased to fa65869284698be152dbe18af2dde0577ee65333.
Attachment #657297 - Attachment is obsolete: true
Attachment #657297 - Flags: review?(jst)
Attachment #657359 - Flags: review?(jst)
(In reply to Alexandre LISSY from comment #30)
> (In reply to Mike Habicher [:mikeh] from comment #29)
> > (In reply to Alexandre LISSY from comment #26)
> > > (In reply to Mike Habicher [:mikeh] from comment #24)
> > > > Created attachment 657239 [details] [diff] [review]
> > > > WIP - cycle collection refactoring of code
> > > > 
> > > > Latest patch, includes bugfixen for camera shutdown leaks.
> > > 
> > > Is it normal that your patch still references types like PRUint64 ?
> > 
> > They're all gone now.
> 
> Nope, see
> https://bugzilla.mozilla.org/attachment.cgi?id=657284&action=diff#a/dom/
> camera/DOMCameraManager.cpp_sec3, on one side you have
> "nsDOMCameraManager::nsDOMCameraManager(PRUint64 aWindowId)" and on the
> other "nsDOMCameraManager::nsDOMCameraManager(uint64_t aWindowId)" but
> current gecko already uses this prototype, see
> https://github.com/mozilla/mozilla-central/blob/master/dom/camera/
> DOMCameraManager.cpp#L33

It should be gone NOW.

Though I do see you a leftover PRUint8 in there that needs removing--FYI, :jst.
Sorry for this, but it ease applying the patch :)
Comment on attachment 657359 [details] [diff] [review]
Cycle collection refactoring of DOM-facing objects, clean-up

Looks good, r=jst!

But I did have a few comments for you to look into before this lands, which are:

- In CameraControlImpl::GetPreviewStream():

+  nsCOMPtr<nsIRunnable> getPreviewStreamTask = new GetPreviewStreamTask(this, aSize, onSuccess, onError);
+  return NS_DispatchToMainThread(getPreviewStreamTask);

Not saying you have to do this, but I think it'd serve us well to have
comments explaining which thread we're runing on in code like this where
more than one thread is used.

And maybe even assert that this code runs on the main thread, or
maybe use NS_DispatchToCurrentThread() for additional clarity? That
way it'd be clear from reading this that this runs on the main thread
and we're simply dispatching a runnable to the event queue to do
something asynchronous.

- In CameraControlImpl::AutoFocus():

+  nsCOMPtr<nsIRunnable> autoFocusTask = new AutoFocusTask(this, onSuccess, onError);
+  return mCameraThread->Dispatch(autoFocusTask, NS_DISPATCH_NORMAL);

This is nice and clear, we're dispatching to a different thread. But
maybe comment (or assert) that this code runs on the main thread?

- In CameraControlImpl::StartPreview()

+  // NS_ENSURE_TRUE(aDOMPreview, NS_ERROR_INVALID_ARG);

You explicitly pass a null aDOMPreview in
nsDOMCameraControl::ResumePreview(), so remove this commented out
check?

- In GetPreviewStreamResult::Run():

+  MOZ_ASSERT(NS_IsMainThread());

Excellent! More asserts like this would be great! :)

+  ~GetPreviewStreamResult()

Given that this inherits nsRunnable and has a virtual method in it,
maybe be explicit and mark the destructor virtual too?

Same thing with ~GetPreviewStreamTask(), ~AutoFocusTask(), and all the
other nsRunnable derivatives here. Not a big deal though, so if it's a
pain to do, no worries... But being explicit about virtual destructors is not a bad idea... It can be a massive pain to debug problems with destructors that should be virtual but aren't.

- In nsDOMCameraControl::GetOnShutter():

+{
+  // TODO: see bug 779138.
   return NS_OK;

Technically you're required to set out params to a sane value if you
return a success code. Here you should probably return
NS_ERROR_NOT_IMPLEMENTED or somesuch.

- In FallbackCameraControl.cpp:

+ * Stub implementation of the DOM-facing camera control constructor.
+ *
+ * This should never get called--it exists to keep the linker happy; if
+ * implemented, it should construct (e.g.) nsFallbackCameraControl and
+ * store a reference in the 'mCameraControl' member (which is why it is
+ * defined here).
+ */
+nsDOMCameraControl::nsDOMCameraControl(uint32_t aCameraId, nsIThread* aCameraThread, nsICameraGetCameraCallback* onSuccess, nsICameraErrorCallback* onError)
+{
+}

If this should never be called, can you make it private and assert
that it never gets called, and inlined in the header? If not, could
you move this into nsDOMCameraControl.cpp with the rest of the
nsDOMCameraControl code?

+++ b/dom/camera/GonkCameraCapabilities.cpp

Should this file be renamed to DOMCameraCapabilities.cpp? Seems
there's little (if anything?) gonk specific here now?

- In GonkCameraControl.cpp:

+// nsDOMCameraControl implementation-specific constructor
+nsDOMCameraControl::nsDOMCameraControl(uint32_t aCameraId, nsIThread* aCameraThread, nsICameraGetCameraCallback* onSuccess, nsICameraErrorCallback* onError)
+  : mDOMCapabilities(nullptr)
+{

Should this not live in nsDOMCameraControl.cpp?

+  mCameraControl = new nsGonkCameraControl(aCameraId, aCameraThread, this, onSuccess, onError);
+
[...]
+  NS_ADDREF_THIS();

This technically violates XPCOM ownership rules in that it passes a
reference to an object to other code before the reference count of the
object has been set to some > 0 value. To avoid that, simply call NS_ADDREF_THIS() before you call the nsGonkCameraControl ctor, and that will avoid problems if that code, or some other code down the road, does an
addref/release call on this object (which would currently cause |this|
to be deleted while in this ctor.

+nsGonkCameraControl::TakePictureComplete(PRUint8* aData, uint32_t aLength)
+{
+  PRUint8* data = new PRUint8[aLength];

s/PRUint8/uint8_t/ here, and in the header as well, as you noted
yourself...

- In GonkNativeWindow::cancelBuffer():

     if (buf < 0 || buf >= mBufferCount) {
         CNW_LOGE("cancelBuffer: slot index out of range [0, %d]: %d",
                 mBufferCount, buf);
         return -EINVAL;
     } else if (mSlots[buf].mBufferState != BufferSlot::DEQUEUED) {
         CNW_LOGE("cancelBuffer: slot %d is not owned by the client (state=%d)",
                 buf, mSlots[buf].mBufferState);
-        return -EINVAL;
+        // return -EINVAL;
+        return OK;

Seems to me we want to return -EINVAL here...
Attachment #657359 - Flags: review?(jst) → review+
And blocking on this as this is needed for the camera to work w/o leaking significantly... It's more than a few objects that are leaked based on talking to Mike...
blocking-basecamp: - → +
(In reply to Johnny Stenback (:jst, jst@mozilla.com) from comment #35)
> Comment on attachment 657359 [details] [diff] [review]
> Cycle collection refactoring of DOM-facing objects, clean-up
> 
> Looks good, r=jst!
> 
> But I did have a few comments for you to look into before this lands, which
> are:
    ...
> - In CameraControlImpl::GetPreviewStream():
> 
> +  nsCOMPtr<nsIRunnable> getPreviewStreamTask = new
> GetPreviewStreamTask(this, aSize, onSuccess, onError);
> +  return NS_DispatchToMainThread(getPreviewStreamTask);
> 
> Not saying you have to do this, but I think it'd serve us well to have
> comments explaining which thread we're running on in code like this where
> more than one thread is used.
> 
> And maybe even assert that this code runs on the main thread, or
> maybe use NS_DispatchToCurrentThread() for additional clarity? That
> way it'd be clear from reading this that this runs on the main thread
> and we're simply dispatching a runnable to the event queue to do
> something asynchronous.

I like that idea of self-documenting code.  I'll do this for GetPreviewStream(), since this is the only function that needs to run on the main thread (since it has to create a DOM-facing CCP there); none of the others need to, even if that's where they normally run from.  

> - In CameraControlImpl::AutoFocus():
> 
> +  nsCOMPtr<nsIRunnable> autoFocusTask = new AutoFocusTask(this, onSuccess,
> onError);
> +  return mCameraThread->Dispatch(autoFocusTask, NS_DISPATCH_NORMAL);
> 
> This is nice and clear, we're dispatching to a different thread. But
> maybe comment (or assert) that this code runs on the main thread?

See above comment.  AutoFocus(), TakePicture(), et al _normally_ run on the main thread, but there's nothing requiring them to do so.

> - In FallbackCameraControl.cpp:
> 
> + * Stub implementation of the DOM-facing camera control constructor.
> + *
> + * This should never get called--it exists to keep the linker happy; if
> + * implemented, it should construct (e.g.) nsFallbackCameraControl and
> + * store a reference in the 'mCameraControl' member (which is why it is
> + * defined here).
> + */
> +nsDOMCameraControl::nsDOMCameraControl(uint32_t aCameraId, nsIThread*
> aCameraThread, nsICameraGetCameraCallback* onSuccess,
> nsICameraErrorCallback* onError)
> +{
> +}
>
> If this should never be called, can you make it private and assert
> that it never gets called, and inlined in the header? If not, could
> you move this into nsDOMCameraControl.cpp with the rest of the
> nsDOMCameraControl code?

I can't make it private or header-inlined, because while the fallback implementation never gets called, when built for B2G devices, the same header is used and the B2G version of this function will be called.  See more comments below.

> - In GonkCameraControl.cpp:
> 
> +// nsDOMCameraControl implementation-specific constructor
> +nsDOMCameraControl::nsDOMCameraControl(uint32_t aCameraId, nsIThread*
> aCameraThread, nsICameraGetCameraCallback* onSuccess,
> nsICameraErrorCallback* onError)
> +  : mDOMCapabilities(nullptr)
> +{
> 
> Should this not live in nsDOMCameraControl.cpp?

Depending on which "platform" we're building for, either Gonk or Fallback, a different version of this function is required.  I agree that I occasionally do a double-take when I open GonkCameraControl.cpp and see nsDOMCameraControl::nsDOMCameraControl at the top, but I can't think of a better way to do this short of creating yet-another platform-specific file.

Thanks for the review!
Created attachment 657979 [details] [diff] [review]
Cycle collection refactoring of DOM-facing objects, clean-up

New version, incorporation feedback from :jst's review.
Attachment #657359 - Attachment is obsolete: true
(In reply to Mike Habicher [:mikeh] from comment #38)
> Created attachment 657979 [details] [diff] [review]
> Cycle collection refactoring of DOM-facing objects, clean-up
> 
> New version, incorporation feedback from :jst's review.

*incorporating.
(-sigh-)
Created attachment 658249 [details] [diff] [review]
Cycle collection refactoring of DOM-facing objects, clean-up

Final version, incorporating non-Gonk build feedback from the try server.

Just waiting for a few... more... greens.
Attachment #656987 - Attachment is obsolete: true
Attachment #657192 - Attachment is obsolete: true
Attachment #657193 - Attachment is obsolete: true
Attachment #657979 - Attachment is obsolete: true
Keywords: checkin-needed
So I guess this is green on Try. Feel free to post a link to your last run next time.

https://hg.mozilla.org/integration/mozilla-inbound/rev/244bba751ce4
Keywords: checkin-needed
(In reply to Ryan VanderMeulen from comment #41)
> So I guess this is green on Try. Feel free to post a link to your last run
> next time.
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/244bba751ce4

Yes, sorry.  Late, but for completeness: https://tbpl.mozilla.org/?tree=Try&rev=6a2bc8094c6c
https://hg.mozilla.org/mozilla-central/rev/244bba751ce4
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Depends on: 822531
Depends on: 988699
You need to log in before you can comment on or make changes to this bug.