779139 - Camera - make cycle collection participants

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Mike Habicher [:mikeh] (high bugzilla latency)]

CameraControl and CameraCapabilities reference each other and need to be made cycle collection participants.

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 2

[Mike Habicher [:mikeh] (high bugzilla latency)]

Attachment: WIP - cycle collection refactoring of code

Comment 3

[Mike Habicher [:mikeh] (high bugzilla latency)]

Attachment: WIP - cycle collection refactoring of code

Comment 4

[Mike Habicher [:mikeh] (high bugzilla latency)]

Attachment: WIP - cycle collection refactoring of code

Comment 5

[Mike Habicher [:mikeh] (high bugzilla latency)]

Attachment: Part 1: DOM-facing cycle-collection participants

Comment 6

[Mike Habicher [:mikeh] (high bugzilla latency)]

Attachment: Part 2: Gonk-facing core implementation

Comment 7

[Mike Habicher [:mikeh] (high bugzilla latency)]

Attachment: WIP - cycle collection refactoring of code

Comment 8

[Mike Habicher [:mikeh] (high bugzilla latency)]

Attachment: Part 2: Gonk-facing core implementation

Comment 9

[Mike Habicher [:mikeh] (high bugzilla latency)]

review? ping.

Comment 10

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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?

Comment 11

[Mike Habicher [:mikeh] (high bugzilla latency)]

Attachment: WIP - cycle collection refactoring of code

Fix up some bit-rot.  This patch should apply cleanly.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 13

[Mike Habicher [:mikeh] (high bugzilla latency)]

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

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

OK, but we use hg here :-).

Comment 15

[Mike Habicher [:mikeh] (high bugzilla latency)]

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

Comment 16

[David Zbarsky (:dzbarsky)]

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?

Comment 17

[Mike Habicher [:mikeh] (high bugzilla latency)]

Attachment: WIP - cycle collection refactoring of code

Rebase properly this time.  /git-rage.

Comment 18

[Mike Habicher [:mikeh] (high bugzilla latency)]

Attachment: WIP - cycle collection refactoring of code

Rebased to latest version of tree.

Comment 19

[Mike Habicher [:mikeh] (high bugzilla latency)]

Attachment: 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.

Comment 20

[Mike Habicher [:mikeh] (high bugzilla latency)]

Attachment: 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.

Comment 21

[:gerard-majax]

Attachment: 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.

Comment 22

[:gerard-majax]

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.

Comment 23

[:gerard-majax]

Attachment: 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 ...

Comment 24

[Mike Habicher [:mikeh] (high bugzilla latency)]

Attachment: WIP - cycle collection refactoring of code

Latest patch, includes bugfixen for camera shutdown leaks.

Comment 25

[Mike Habicher [:mikeh] (high bugzilla latency)]

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

Comment 26

[:gerard-majax]

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

Comment 27

[Mike Habicher [:mikeh] (high bugzilla latency)]

Attachment: WIP - cycle collection refactoring of code

A final bugfix to make sure we don't call abandon() on a non-existant window.

Comment 28

[Mike Habicher [:mikeh] (high bugzilla latency)]

Attachment: WIP - cycle collection refactoring of code

Includes all previous changes, and fixes the non-debug build.

Comment 29

[Mike Habicher [:mikeh] (high bugzilla latency)]

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

Comment 30

[:gerard-majax]

(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

Comment 31

[Mike Habicher [:mikeh] (high bugzilla latency)]

Attachment: 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.

Comment 32

[Mike Habicher [:mikeh] (high bugzilla latency)]

Attachment: Cycle collection refactoring of DOM-facing objects, clean-up

Rebased to fa65869284698be152dbe18af2dde0577ee65333.

Comment 33

[Mike Habicher [:mikeh] (high bugzilla latency)]

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

Comment 34

[:gerard-majax]

Sorry for this, but it ease applying the patch :)

Comment 35

[Johnny Stenback (:jst)]

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

Comment 36

[Johnny Stenback (:jst)]

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

Comment 37

[Mike Habicher [:mikeh] (high bugzilla latency)]

(In reply to Johnny Stenback (:jst, [e-mail]) 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!

Comment 38

[Mike Habicher [:mikeh] (high bugzilla latency)]

Attachment: Cycle collection refactoring of DOM-facing objects, clean-up

New version, incorporation feedback from :jst's review.

Comment 39

[Mike Habicher [:mikeh] (high bugzilla latency)]

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

Comment 40

[Mike Habicher [:mikeh] (high bugzilla latency)]

Attachment: 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.

Comment 41

[Ryan VanderMeulen [:RyanVM]]

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

Comment 42

[Mike Habicher [:mikeh] (high bugzilla latency)]

(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

Comment 43

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/244bba751ce4