1086711 - [B2G][Camera][Gecko] Reduce camera push parameter thrashing

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: Gaia::Camera, defect)

Description

[Andrew Osmond [:aosmond] (he/him)]

When the application sets camera parameters, it frequently sets them in batches, although the gecko camera layer and driver do not know this. Each set currently triggers a dispatch to the camera thread to push all of the current parameters to the drivers (including unchanged ones, requirement to function of many drivers). Typically each dispatch is not handled immediately, and when the first dispatch is processed, it occurs after the last application-set parameter. Thus the subsequent dispatches are not required but happen anyways.

We should regulate push parameters, such that if there is a pending dispatch, we don't queue another, and the camera thread itself forces a push due to its own state machine, it should discard the extra pending when it happens.

Comment 1

[Andrew Osmond [:aosmond] (he/him)]

Attachment: bug1086711.patch, v1

Comment 2

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

*yoink*

Comment 3

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

Attachment: WIP - (a different approach to) reduce parameter thrashing, v1

Comment 4

[Andrew Osmond [:aosmond] (he/him)]

Comment on attachment 8509755 [details] [diff] [review]
WIP - (a different approach to) reduce parameter thrashing, v1

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

Approach looks fine to me.

::: dom/camera/GonkCameraControl.cpp
@@ +359,3 @@
>     */
> +  if (NS_GetCurrentThread() == mCameraThread) {
> +    return PushParametersImpl();

In normal operation I do see this push handling queued pushes waiting in the message queue. I think we should use/update mConfigPushSequence to ignore those waiting in the message queue and trigger the callback.

Comment 5

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

Attachment: Reduce parameter thrashing, v2

I've tried to rationalize the names of the various low-level parameter-pushing and -pulling functions. Please let me know if you think there's a way that this could be made more clear.

try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=60c000ffb19a

Comment 6

[Andrew Osmond [:aosmond] (he/him)]

Comment on attachment 8512282 [details] [diff] [review]
Reduce parameter thrashing, v2

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

r+ conditional on nits and fixing that bug.

::: dom/camera/GonkCameraControl.cpp
@@ +343,5 @@
> +nsGonkCameraControl::GetParameters()
> +{
> +  DOM_CAMERA_LOGI("Getting camera parameters\n");
> +  MOZ_ASSERT(NS_GetCurrentThread() == mCameraThread);
> +  RETURN_IF_NO_CAMERA_HW();

Perhaps we should assert that get happens with mPendingFlushSetParametersSequence == 0 and mDeferFlushSetParameters == 0?

@@ +367,5 @@
> +    return rv;
> +  }
> +  if (mPendingFlushSetParametersSequence.compareExchange(0, 0)) {
> +    OnSettingsChanged(sequence);
> +  }

Do you need to do a compare exchange here? Shouldn't a compare be enough? (I assume the read from the atomic value is also atomic.)

@@ +472,3 @@
>    if (dcu == 0) {
>      NS_WARNING("Underflow badness decrementing mDeferConfigUpdate!");
>      MOZ_CRASH();

We need the ability to send/collect user error reports without crashing. I recall getting yelled at in a different life for crashing as a diagnostic :).

@@ +679,5 @@
>      return NS_ERROR_FAILURE;
>    }
>  
>    OnPreviewStateChange(CameraControlListener::kPreviewStarted);
>    return NS_OK;

We can still do NS_WARN_IF(NS_FAILED(rv)), but I think we should return rv itself instead of clamping it. There are several occurrences like this in the file.

::: dom/camera/GonkCameraHwMgr.cpp
@@ +358,5 @@
> +      return NS_ERROR_NOT_IMPLEMENTED;
> +
> +    default:
> +      DOM_CAMERA_LOGE("Start face detection failed with status %d", rv);
> +      return NS_ERROR_FAILURE;

Print is wrong here. Should be stop face detection.

::: dom/camera/GonkCameraSource.cpp
@@ +455,5 @@
>      status_t err = OK;
>      //TODO: need to do something here to check the sanity of camera
>  
>      CameraParameters params;
> +    mCameraHw->SetParameters(params);

This should be a get, not a set.

Comment 7

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

Attachment: Reduce parameter thrashing, v2.1 r=aosmond

Clean-up, fix non-Gonk bustage.

try push: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=84de468dbe7a

Comment 8

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

(In reply to Andrew Osmond [:aosmond] from comment #6)
> Comment on attachment 8512282 [details] [diff] [review]
> Reduce parameter thrashing, v2
> 
> Review of attachment 8512282 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ conditional on nits and fixing that bug.
> 
> ::: dom/camera/GonkCameraControl.cpp
> @@ +343,5 @@
> > +nsGonkCameraControl::GetParameters()
> > +{
> > +  DOM_CAMERA_LOGI("Getting camera parameters\n");
> > +  MOZ_ASSERT(NS_GetCurrentThread() == mCameraThread);
> > +  RETURN_IF_NO_CAMERA_HW();
> 
> Perhaps we should assert that get happens with
> mPendingFlushSetParametersSequence == 0 and mDeferFlushSetParameters == 0?

I don't think we can guarantee mPendingFlushSetParametersSequence == 0. I also think you've identified a potential race condition in my patch: it's possible to call UpdateLocalParameters() with pending sets, trigger the FlushAllSetParameters() and SetParameters() calls, then have additional Set()s occur before the GetParameters() call occurs and wipes out those Set()s.

I need to rethink this a bit.

> @@ +472,3 @@
> >    if (dcu == 0) {
> >      NS_WARNING("Underflow badness decrementing mDeferConfigUpdate!");
> >      MOZ_CRASH();
> 
> We need the ability to send/collect user error reports without crashing. I
> recall getting yelled at in a different life for crashing as a diagnostic :).

That was then. In this case, between the crash reporter and gdb, I think triggering a crash -- especially in this case, where there's no sane way to proceed -- is a) attention-getting, and b) the best way to gather forensic data.

Comment 9

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

Attachment: WIP - reduce parameter thrashing, v2.2

Rebased. Dropping r+ for now.