Closed Bug 965421 Opened 10 years ago Closed 10 years ago

[Madai][Camera][Gecko][jb][kk] Implement continuous auto-focus status callback

Categories

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

Product:
Firefox OS Graveyard
Component:
Gaia::Camera
Platform:
ARM
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
1.4 S5 (11apr)

People

(Reporter: mikeh, Assigned: youngwoo.jo)

References

Details

(Keywords: dev-doc-needed)

Attachments

(1 file, 5 obsolete files)

AutoFocusMovingCallback-API.patch, v1.5
29.55 KB, patch
mikeh
: review+
Details | Diff | Splinter Review 
AOSP 4.2 added a callback to indicate the status of the continuous auto-focus process:
http://developer.android.com/reference/android/hardware/Camera.AutoFocusMoveCallback.html

The callback gets a boolean that indicates whether or not the camera is currently moving the focus position.
Depends on: 909542
I didn't know this callback is needed.
So now I am checking this feature.
And now I found the notification from camera HAL layer.

CAMERA_MSG_FOCUS notification is for AutoFocusCallback.
Current camera gonk handle this message and CAMERA_MSG_SHUTTER.
In addition to those notification, we need to handle AutoFocusMoveCallback for C-AF.
This is notified with CAMERA_MSG_FOCUS_MOVE notification.
So I expect that if we handle this, it seems to support this callback to Gaia.
I will test it with CAMERA_MSG_FOCUS_MOVE.

So I think it needs two APIs like the below Android APIs.
 - setAutoFocusMoveCallback (Camera.AutoFocusMoveCallback cb) : for setting callback
 - Camera.AutoFocusMoveCallback : called when CAMERA_MSG_FOCUS_MOVE is notified
Youngwoo, have you started implementing this callback? If not, I will implement it.
Status: NEW → ASSIGNED
No, I have not started implementing this.
You may take this issue.
blocking-b2g: 1.4? → ---
Summary: [Camera][Gecko][jb][kk] Implement continuous auto-focus status callback → [Madai][Camera][Gecko][jb][kk] Implement continuous auto-focus status callback
Target Milestone: --- → 1.4 S2 (28feb)
Based on the call on 2/11, it looks like this is already being implemented. Youngwoo, please provide the patch for mike to review.

Thanks
Hema
Attached patch AutoFocusMoveCallback-API.patch (obsolete) — — Splinter Review 
I added the below API to provide the notification of auto-focus-moving.

callback CameraAutoFocusMoveCallback = void (boolean started);
interface CameraControl
{
  [SetterThrows]
  attribute CameraAutoFocusMoveCallback? onAutoFocusMove;
}

I did not added setAutoFocusCallback method. Instead of set method, I used only one onAutoFocusMove callback attribute.
And in order to receive the notification from camera library, it has to set the command for starting/stopping notification to camera library.
When the app-side sets onAutoFocusMove, it sends the command for starting the notification to camera library.
If onAutoFocusMove is set to null by the app-side, it sends the command for stopping the notification.

onAutoFocus
If this API is used with autoFocus(CameraAutoFocusCallback onSuccess) API, it makes a crash.
Basically, CameraAutoFocusCallback is called only one time.
However, in this case, CameraAutoFocusCallback is called multiple times from camera library.
In the first call to CameraAutoFocusCallback from camera library, nsDOMCameraControl sets the member variable for the callback to null, because it's an one time callback.
In the second call to it, nsDOMCameraControl tries to call the callback using the callback member variable, without the null-check to it.
So it makes a crash.

I've already filed it in Bug 976915.
This issue should be resolved first.
It's already resolved by Bug 976032.
So Bug 976915 is duplicated.
Comment on attachment 8381955 [details] [diff] [review]
AutoFocusMoveCallback-API.patch

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

Although it appears at the end of the file list, please start with my comments on the .webidl change.

::: dom/camera/CameraControlImpl.cpp
@@ +120,5 @@
>    }
>  }
>  
>  void
> +CameraControlImpl::OnAutoFocusMoving(bool aAutoFocusMove)

"aIsMoving"

@@ +415,5 @@
>      new Message(this, CameraControlListener::kInAutoFocus, aCancelExistingCall), NS_DISPATCH_NORMAL);
>  }
>  
>  nsresult
> +CameraControlImpl::SetEnableAutoFocusMove(bool aEnable)

Not required with WebIDL change.

::: dom/camera/CameraControlImpl.h
@@ +42,5 @@
>    virtual nsresult SetConfiguration(const Configuration& aConfig) MOZ_OVERRIDE;
>    virtual nsresult StartPreview() MOZ_OVERRIDE;
>    virtual nsresult StopPreview() MOZ_OVERRIDE;
>    virtual nsresult AutoFocus(bool aCancelExistingCall) MOZ_OVERRIDE;
> +  virtual nsresult SetEnableAutoFocusMove(bool aEnable) MOZ_OVERRIDE;

Not required with WebIDL change.

@@ +61,5 @@
>  
>  protected:
>    // Event handlers.
>    void OnAutoFocusComplete(bool aAutoFocusSucceeded);
> +  void OnAutoFocusMoving(bool aAutoFocusMove);

"aIsMoving"

@@ +99,5 @@
>    virtual nsresult SetConfigurationImpl(const Configuration& aConfig) = 0;
>    virtual nsresult StartPreviewImpl() = 0;
>    virtual nsresult StopPreviewImpl() = 0;
>    virtual nsresult AutoFocusImpl(bool aCancelExistingCall) = 0;
> +  virtual nsresult SetEnableAutoFocusMoveImpl(bool aEnable) = 0;

Not required with WebIDL change.

::: dom/camera/CameraControlListener.h
@@ +75,5 @@
>    virtual void OnConfigurationChange(const CameraListenerConfiguration& aConfiguration) { }
>  
>    virtual void OnAutoFocusComplete(bool aAutoFocusSucceeded) { }
>    virtual void OnTakePictureComplete(uint8_t* aData, uint32_t aLength, const nsAString& aMimeType) { }
> +  virtual void OnAutoFocusMoving(bool aAutoFocusMove) { }

"aIsMoving"

@@ +82,5 @@
>    {
>      kInStartCamera,
>      kInStopCamera,
>      kInAutoFocus,
> +    kInSetEnableAutoFocusMove,

No longer required.

::: dom/camera/DOMCameraControl.cpp
@@ +693,5 @@
> +{
> +  MOZ_ASSERT(mCameraControl);
> +
> +  mOnAutoFocusMoveCb = aCb;
> +  aRv = mCameraControl->SetEnableAutoFocusMove( aCb ? 1 : 0 );

Change this to reflect the WebIDL change.

@@ +1165,5 @@
>    cb->Call(aAutoFocusSucceeded, ignored);
>  }
>  
>  void
> +nsDOMCameraControl::OnAutoFocusMoving(bool aAutoFocusMovingStarted)

"aIsMoving"

::: dom/camera/DOMCameraControl.h
@@ +143,5 @@
>  
>    void OnCreatedFileDescriptor(bool aSucceeded);
>  
>    void OnAutoFocusComplete(bool aAutoFocusSucceeded);
> +  void OnAutoFocusMoving(bool aAutoFocusMovingStarted);

"aIsMoving"

::: dom/camera/DOMCameraControlListener.cpp
@@ +218,5 @@
>    NS_DispatchToMainThread(new Callback(mDOMCameraControl, aConfiguration));
>  }
>  
>  void
> +DOMCameraControlListener::OnAutoFocusMoving(bool aAutoFocusMovingStarted)

"aIsMoving"

@@ +235,5 @@
> +      aDOMCameraControl->OnAutoFocusMoving(mAutoFocusMovingStarted);
> +    }
> +
> +  protected:
> +    bool mAutoFocusMovingStarted;

"mIsMoving"

::: dom/camera/DOMCameraControlListener.h
@@ +18,5 @@
>  public:
>    DOMCameraControlListener(nsDOMCameraControl* aDOMCameraControl, CameraPreviewMediaStream* aStream);
>  
>    virtual void OnAutoFocusComplete(bool aAutoFocusSucceeded) MOZ_OVERRIDE;
> +  virtual void OnAutoFocusMoving(bool aAutoFocusMovingStarted) MOZ_OVERRIDE;

"aIsMoving"

::: dom/camera/FallbackCameraControl.cpp
@@ +24,5 @@
>  public:
>    FallbackCameraControl(uint32_t aCameraId) : CameraControlImpl(aCameraId) { }
>  
>    void OnAutoFocusComplete(bool aSuccess);
> +  void OnAutoFocusMoving(bool aStart) { }

"aStart" --> "aIsMoving"

@@ +60,5 @@
>  
>    virtual nsresult StartPreviewImpl() { return NS_ERROR_FAILURE; }
>    virtual nsresult StopPreviewImpl() { return NS_ERROR_FAILURE; }
>    virtual nsresult AutoFocusImpl(bool aCancelExistingCall) { return NS_ERROR_FAILURE; }
> +  virtual nsresult SetEnableAutoFocusMoveImpl(bool aEnable) { return NS_ERROR_FAILURE; }

Remove due to WebIDL change.

::: dom/camera/GonkCameraControl.cpp
@@ +545,5 @@
>    return NS_OK;
>  }
>  
>  nsresult
> +nsGonkCameraControl::SetEnableAutoFocusMoveImpl(bool aEnable)

With the WebIDL change, this method is not required.

@@ +964,5 @@
>    mCameraThread->Dispatch(new AutoFocusComplete(this, aSuccess), NS_DISPATCH_NORMAL);
>  }
>  
>  void
> +nsGonkCameraControl::OnAutoFocusMoving(bool aStart)

This method is not required. Just call the base class member directly.

@@ +1420,5 @@
>  
>  void
> +OnAutoFocusMoving(nsGonkCameraControl* gc, bool aStart)
> +{
> +  gc->OnAutoFocusMoving(aStart);

"aStart" --> "aIsMoving"

::: dom/camera/GonkCameraControl.h
@@ +48,5 @@
>  public:
>    nsGonkCameraControl(uint32_t aCameraId);
>  
>    void OnAutoFocusComplete(bool aSuccess);
> +  void OnAutoFocusMoving(bool aStart);

"aStart" --> "aIsMoving"

@@ +105,5 @@
>  
>    virtual nsresult StartPreviewImpl() MOZ_OVERRIDE;
>    virtual nsresult StopPreviewImpl() MOZ_OVERRIDE;
>    virtual nsresult AutoFocusImpl(bool aCancelExistingCall) MOZ_OVERRIDE;
> +  virtual nsresult SetEnableAutoFocusMoveImpl(bool aEnable) MOZ_OVERRIDE;

With the WebIDL change, this method is no longer required.

@@ +165,5 @@
>  // camera driver callbacks
>  void OnTakePictureComplete(nsGonkCameraControl* gc, uint8_t* aData, uint32_t aLength);
>  void OnTakePictureError(nsGonkCameraControl* gc);
>  void OnAutoFocusComplete(nsGonkCameraControl* gc, bool aSuccess);
> +void OnAutoFocusMoving(nsGonkCameraControl* gc, bool aStart);

"aStart" --> "aIsMoving"

::: dom/camera/GonkCameraHwMgr.cpp
@@ +282,5 @@
> +{
> +  DOM_CAMERA_LOGI("%s\n", __func__);
> +  int rv = OK;
> +
> +  rv = mCamera->sendCommand(CAMERA_CMD_ENABLE_FOCUS_MOVE_MSG, (int)aEnable, 0);

With the WebIDL change, this method is no longer required. Just set CAMERA_CMD_ENABLE_FOCUS_MOVE_MSG by default.

Note that this will require a conditional compilation guard since CAMERA_CMD_ENABLE_FOCUS_MOVE_MSG was not introduced until JB/4.1.1, and we need to continue to support ICS/4.0.4.

Also, please use C++-style casts.

#if ANDROID_VERSION >= 16
  rv = mCamera->sendCommand(CAMERA_CMD_ENABLE_FOCUS_MOVE_MSG, static_cast<int>(aEnable), 0);
#endif

::: dom/camera/GonkCameraHwMgr.h
@@ +78,5 @@
>    virtual int      GetSensorOrientation(uint32_t aType = RAW_SENSOR_ORIENTATION);
>  
>    virtual int      AutoFocus();
>    virtual void     CancelAutoFocus();
> +  virtual int      SetEnableAutoFocusMove(bool aEnable);

With the WebIDL change, this method is no longer required.

::: dom/camera/ICameraControl.h
@@ +133,5 @@
>    virtual nsresult TakePicture() = 0;
>    virtual nsresult StartRecording(DeviceStorageFileDescriptor *aFileDescriptor,
>                                    const StartRecordingOptions* aOptions = nullptr) = 0;
>    virtual nsresult StopRecording() = 0;
> +  virtual nsresult SetEnableAutoFocusMove(bool aEnable) = 0;

With the WebIDL change, this method is no longer required.

::: dom/webidl/CameraControl.webidl
@@ +127,5 @@
>  callback CameraClosedCallback = void ();
>  callback CameraReleaseCallback = void ();
>  callback CameraRecorderStateChange = void (DOMString newState);
>  callback CameraPreviewStateChange = void (DOMString newState);
> +callback CameraAutoFocusMoveCallback = void (boolean started);

"started" --> "isMoving"

@@ +280,5 @@
> +     This is only supported in continuous autofocus modes; continuous auto focus
> +     picture mode and continuous auto focus video mode. Applications can
> +     show autofocus animation based on this. */
> +  [SetterThrows]
> +  attribute CameraAutoFocusMoveCallback? onAutoFocusMove;

Don't make the enabling of this feature conditional on this callback being set. Just make the callback a simple attribute, and whether or not it gets called dependent on .focusMode = "continuous-picture" || "continuous-video". With that change, you can also remove "[SetterThrows]".
Assignee: mhabicher → hiro7998
I checked how it's implemented in android.
The below is the comparison between my patch and Android's

(1) The code similar with onAutoFocusMove in DOMCameraControl.cpp
My patch : nsDOMCameraControl::SetOnAutoFocusMove() in DOMCameraControl.cpp
Android  : http://androidxref.com/4.4.2_r1/xref/frameworks/base/core/java/android/hardware/Camera.java#1141

Android is implemented similar with my patch. When the callback is set by camera app, it checks if callback is set or not and then it sends the command for enable/disable to camera library.
 
if it sends CAMERA_CMD_ENABLE_FOCUS_MOVE_MSG with '0' value, camera library sends the notification continuously. However, if camera app doesn't set the callback, they are just the unnecessary notifications.
In my opinion, setting CAMERA_CMD_ENABLE_FOCUS_MOVE_MSG by default is not good.
If you think that onAutoFocusMove callback should be a simple attribute, I suggest we add another method for making it enable/disable.
If you think like that, I will apply it to the patch.

Mikeh, What do you think about this?
Youngwoo, the Android implementation is fine because Camera.java is their app. This bug is about a service-level API, and I do not want to artificially couple enabling continuous-focus mode with whether or not there is a callback assigned.

Regardless, whether or not the callback is enabled can't be done in the DOM layer, because users of ICameraControl/CameraControlListener may want to use continuous focus as well.

If CAMERA_CMD_ENABLE_FOCUS_MOVE_MSG is always enabled, how often is the CAMERA_MSG_FOCUS_MOVE message sent? I assume if .focusMode != "continuous-picture" || "continuous-video" then the message is not sent at all.
I talked with a camera developer. He said the overhead of notification messages is few and, though CAMERA_CMD_ENABLE_FOCUS_MOVE_MSG is enabled or disabled, it affects only whether the notification will be passed or not, but not affect camera's internal routine.
So I agree with you. I will apply it.
This patch is doing only set/get the callback in DOM layer and sending CAMERA_CMD_ENABLE_FOCUS_MOVE_MSG with '1' value to camera library on initializing CameraHardare.
Attachment #8381955 - Attachment is obsolete: true
Comment on attachment 8383382 [details] [diff] [review]
AutoFocusMoveCallback-API.patch, v1.2

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

This looks excellent. Please address the issues I've identified below and provide automated tests.

The continuous integration tests are run on an emulator which implements only basic camera features; bug 976802 implements the ability to inject fake camera parameters for testing, so we can pretend to support "continuous-picture" and "continuous-video". We can also detect when one of these is set to allow TestGonkCameraHardware.cpp/.h to supply fake OnAutoFocusMove() events.

::: dom/camera/CameraControlImpl.h
@@ +57,5 @@
>    void OnClosed();
>    void OnError(CameraControlListener::CameraErrorContext aContext,
>                 CameraControlListener::CameraError aError);
>  
> +  // We make this public for a direct call

I like this comment. Please move it above OnShutter() above and change it to "Event handlers called directly from outside this class".

::: dom/camera/DOMCameraControl.cpp
@@ +692,5 @@
> +void
> +nsDOMCameraControl::SetOnAutoFocusMove(CameraAutoFocusMoveCallback* aCb)
> +{
> +  MOZ_ASSERT(mCameraControl);
> +

Please remove the assertion (and the blank line)--it is not required for this method.

::: dom/camera/GonkCameraHwMgr.cpp
@@ +97,5 @@
>        break;
>  
> +    case CAMERA_MSG_FOCUS_MOVE:
> +      OnAutoFocusMoving(mTarget, !!ext1);
> +      break;

This block also needs to be protected by #if ANDROID_VERSION >= 16

@@ +178,5 @@
>    mCamera->setPreviewTexture(mNativeWindow);
>  #endif
>  
> +#if ANDROID_VERSION >= 16
> +  rv = mCamera->sendCommand(CAMERA_CMD_ENABLE_FOCUS_MOVE_MSG, 1, 0);

If you're going to grab the return value, you should do something with it. How about:
  if (NS_FAILED(rv)) {
    NS_WARNING("Failed to enable CAMERA_MSG_FOCUS_MOVE, might miss continuous autofocus events");
  }

::: dom/webidl/CameraControl.webidl
@@ +278,5 @@
>  
> +  /* Callback used to notify on auto focus start and stop.
> +     This is only supported in continuous autofocus modes; continuous auto focus
> +     picture mode and continuous auto focus video mode. Applications can
> +     show autofocus animation based on this. */

Please change the text to, "if continuous autofocus is supported and focusMode is set to enable it, then this function is called whenever the camera decides to start and stop moving the focus position; it can be used to update a UI element to indicate that the camera is still trying to focus, or has finished. Some platforms do not support this event, in which case the callback is never invoked."

@@ +279,5 @@
> +  /* Callback used to notify on auto focus start and stop.
> +     This is only supported in continuous autofocus modes; continuous auto focus
> +     picture mode and continuous auto focus video mode. Applications can
> +     show autofocus animation based on this. */
> +  attribute CameraAutoFocusMoveCallback? onAutoFocusMove;

It looks like this is called Moving instead of Move everywhere else. Let's change this to Moving as well to be consistent.
please provide eta on updated PR with review comments addressed
Flags: needinfo?(hiro7998)
I modified the patch by your comments. however I've not yet finished implementing a test script. I will also upload it soon.
Attachment #8383382 - Attachment is obsolete: true
Flags: needinfo?(hiro7998)
Target Milestone: 1.4 S2 (28feb) → 1.4 S3 (14mar)
Comment on attachment 8386087 [details] [diff] [review]
AutoFocusMoveCallback-API.patch, v1.3

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

This needs a few cosmetic changes, but otherwise this looks ready to land as soon as you have automated tests written. Great work!

::: dom/camera/CameraControlImpl.h
@@ +57,5 @@
>    void OnClosed();
>    void OnError(CameraControlListener::CameraErrorContext aContext,
>                 CameraControlListener::CameraError aError);
>  
> +  // Event handlers called directly from outside this class.

Move this comment above OnShutter() since it applies to all of these member functions, and remove the blank line between OnError() and OnAutoFocusMoving().

::: dom/camera/DOMCameraControl.cpp
@@ +61,5 @@
>                                        mOnShutterCb,
>                                        mOnClosedCb,
>                                        mOnRecorderStateChangeCb,
> +                                      mOnPreviewStateChangeCb,
> +                                      mOnAutoFocusMovingCb)

This is going to conflict with your patch for face detection. :)

@@ +657,5 @@
> +{
> +  nsCOMPtr<CameraAutoFocusMovingCallback> cb = mOnAutoFocusMovingCb;
> +  return cb.forget();
> +}
> +

Remove this blank line.

::: dom/camera/GonkCameraHwMgr.cpp
@@ +180,5 @@
>    mCamera->setPreviewTexture(mNativeWindow);
>  #endif
>  
> +#if ANDROID_VERSION >= 16
> +  rv = mCamera->sendCommand(CAMERA_CMD_ENABLE_FOCUS_MOVE_MSG, 1, 0);

Please add a warning if this command fails:
  if (NS_FAILED(rv)) {
    NS_WARNING("Failed to send command CAMERA_CMD_ENABLE_FOCUS_MOVE_MSG");
  }

Just in case. The camera may still be usable even if this fails, but I don't want the failure to be hidden.
(In reply to Mike Habicher [:mikeh] from comment #16)
> ::: dom/camera/DOMCameraControl.cpp
> @@ +61,5 @@
> >                                        mOnShutterCb,
> >                                        mOnClosedCb,
> >                                        mOnRecorderStateChangeCb,
> > +                                      mOnPreviewStateChangeCb,
> > +                                      mOnAutoFocusMovingCb)
> 
> This is going to conflict with your patch for face detection. :)
> 
How can I make the patch to remove the conflict between my patches?
Should I set the dependency between the bugs and should I also make the patch of the blocked bug based on the temporary patch of the dependent bug?

Can you let me know a good way to resolve the conflict between the patches?
Flags: needinfo?(mhabicher)
I modified the patch by following Mikeh's comments.
Attachment #8386087 - Attachment is obsolete: true
I'm writing a test in mochitest. Is mochitest enough to contribute the patch to the Mozilla Central?
Or does it require more tests?
Mikeh, Could you let me know about all kinds of the tests required for the contribution?
And also, please reply the needinfo in the comment 17.
Comment on attachment 8386557 [details] [diff] [review]
AutoFocusMoveCallback-API.patch, v1.4

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

This looks ready to go, once the extra preprocessor macro is removed from nsHttpHandler.cpp.

jst, can you review the DOM-facing changes in:
- CameraControl.webidl
- DOMCamera*.cpp/.h
...?

::: netwerk/protocol/http/nsHttpHandler.cpp
@@ +685,5 @@
>      else
>          mCompatDevice.AssignLiteral("Mobile");
>  #endif
>  
> +#if 0 //defined(MOZ_WIDGET_GONK)

I think this change was unintentionally included in your patch.
Attachment #8386557 - Flags: review?(jst)
(In reply to Youngwoo Jo from comment #17)
>
> How can I make the patch to remove the conflict between my patches?

I just meant that whichever patch lands first, the other patch will need you to add/use NS_IMPL_CYCLE_COLLECTION_INHERITED_20.

(In reply to Youngwoo Jo from comment #19)
>
> I'm writing a test in mochitest. Is mochitest enough to contribute the patch
> to the Mozilla Central?

Mochitests are fine. Please try to think about ways to fake the events since the emulator camera doesn't support autofocus events.
Flags: needinfo?(mhabicher)
Comment on attachment 8386557 [details] [diff] [review]
AutoFocusMoveCallback-API.patch, v1.4

Looks good to me too, with the exception of the seemingly unintentional #if 0 block. r=jst with that removed from the patch.
Attachment #8386557 - Flags: review?(jst) → review+
I removed the redundant code. I'm sorry for my mistake.
I'm currently writing the test now.
Soon I'll upload it.
Attachment #8386557 - Attachment is obsolete: true
I uploaded the test script.
Attachment #8387618 - Attachment is obsolete: true
(In reply to Youngwoo Jo from comment #24)
>
> Created attachment 8388062 [details] [diff] [review]
> AutoFocusMovingCallback-API.patch, v1.5
> 
> I uploaded the test script.

I just took a quick look at this and it looks good. I'll do a more thorough review later.
Comment on attachment 8388062 [details] [diff] [review]
AutoFocusMovingCallback-API.patch, v1.5

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

I think this looks good.

try-server push: https://tbpl.mozilla.org/?tree=Try&rev=33e35d2a95dd&showall=1
Attachment #8388062 - Flags: review+
I found a problem from this patch, which is making a focus ring blinked in a camera app.

The problem is, the ‘false’ result from a camera library looks being injected into the sequence of ‘true’, very often, for a very short term (about 1 millisecond).
As a result, camera app is blinking in some cases.

e.g.
App's UI implementation:
  camera app shows a green ring for ‘true’ and a gray ring for ‘false’

The current:
  true, ..(for a long duration) => false (for a 1ms) => true, ..(for a long duration) => false (for a 1ms) => true, ..
  ==> Issue : So, the focus ring is blinking in the UI, because of the very short ‘false’s

The normal expected:
  true, true, .. (for a long duration) => false (for a long duration) => true, true, .. (for a long duration) => false (for a long duration) => true, true, ..
  ==> No blinking

My first patch in this bug does not have a problem like this, but the other versions in this bug have this problem. Mikeh, As you know, the first patch sends the enable/disable command to the camera library, when a camera app sets a callback. And its implementation is same as Android’s

This issue looks dependent on the camera library’s implementation. I found the same issue from my AOSP nexus 4 and from my another device.
The issue is happening only when focus mode is set to 'continuous-picture' or 'continuous-video', not in case of 'auto' focus mode.
I'm sorry that my problem which I found is detected by my mistake.
Camera app developer reported it to me and then, when I'd tested it, it looked like a problem.
However, I've collected the result from the several tests and they have the almost same result between the problem version and normal version which I said it's normal.
As a result, this patch look working fine.
So, I apologize that I made you confused about that.
https://hg.mozilla.org/mozilla-central/rev/3f481b3c3e71
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: 1.4 S3 (14mar) → 1.4 S5 (11apr)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: