909542 - Camera Control API clean-up/streamline

Status: RESOLVED FIXED

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

Description

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

Clean up and streamline the Camera Control API. It's grown crufty as we've learned how to use the underlying AOSP.

Comment 1

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

Attachment: WIP - clean-up of the CameraControl API, v1

This doesn't build--still need to resolve some XPCOMisms.

The patch does a couple of things:
- consolidates the naming conventions in the code
- decouples the DOM/JS-facing objects from the underlying implementation
- does away with the separate preview object (want to add DOMMediaStream to DOMCameraControl)
- simplifies a lot of the implementation

TODO:
- figure out how to make nsDOMCameraControl also be a DOMMediaStream
- figure out how to get nsProxyRelease to choose an nsISupports base
- finish implementing DOM-facing API

Comment 2

[Marco Chen [:mchen]]

Hi Mike,

Except to make nsDOMCameraControl also be a DOMMediaStream, may I know you will use gUM to get Camera's DOMMediaStream or still use mozCamera? Thanks.

Comment 3

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

Hi Marco, for now I'm thinking we'll continue to use mozCamera, but part of my goal with this patch is to make is easier to ultimately transition to getUserMedia().

Comment 4

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

Attachment: WIP - clean-up of the CameraControl API, v2

This _almost_ builds. The only error left is:

In file included from ../../dist/include/nsThreadUtils.h:19,
                 from /home/mikeh/dev/mozilla/m-c/src/dom/camera/DOMCameraControlListener.cpp:6:
../../dist/include/nsCOMPtr.h: In constructor 'nsCOMPtr<T>::nsCOMPtr(nsQueryInterface) [with T = mozilla::nsDOMCameraControl]':
../../dist/include/nsCOMPtr.h:524:   instantiated from 'void nsCOMPtr<T>::Assert_NoQueryNeeded() [with T = mozilla::nsDOMCameraControl]'
../../dist/include/nsCOMPtr.h:560:   instantiated from 'nsCOMPtr<T>::nsCOMPtr(T*) [with T = mozilla::nsDOMCameraControl]'
/home/mikeh/dev/mozilla/m-c/src/dom/camera/DOMCameraControlListener.cpp:30:   instantiated from here
../../dist/include/nsCOMPtr.h:588: error: no class template named 'COMTypeInfo' in 'class mozilla::nsDOMCameraControl'

It's not immediately clear how to get this class template into my class.

Comment 5

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

Attachment: WIP - clean-up of the CameraControl API, v3

This builds. (Or at least, my camera control changes don't break anything.)

It turns out the build error with the previous patch was a result of trying to put the nsDOMCameraControl object in DOMCameraControlListener.cpp into an nsCOMPtr instead of an nsRefPtr.

Thank you, bug 391275 comment 27!

Comment 6

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

Attachment: WIP - clean-up of the CameraControl API, v4

This builds (and links). Still untested, and some of the DOMCameraControl hooks are missing.

Comment 7

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

Attachment: WIP - clean-up of the CameraControl API, v5

Builds, gets the camera and starts the preview, but the viewfinder is black.

Comment 8

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

Attachment: WIP - clean-up of the CameraControl API, v6

Finally sorted out the 'Permission denied to create wrapper for object of class UnnamedClass' error. All that was missing was an interface map for the DOMCameraConfiguration class in DOMCameraControl.cpp:

NS_INTERFACE_MAP_BEGIN(DOMCameraConfiguration)
  NS_INTERFACE_MAP_ENTRY(nsISupports)
  NS_INTERFACE_MAP_ENTRY(nsICameraConfiguration)
  NS_DOM_INTERFACE_MAP_ENTRY_CLASSINFO(CameraConfiguration)
NS_INTERFACE_MAP_END

(I call this out here, because I solved this once before, and promptly forgot how.)

This builds and runs and doesn't crash, though the viewfinder is still black. logcat shows:

[JavaScript Error: "TypeError: this.gotPreviewScreen is not a function" {file: "app://camera.gaiamobile.org/js/camera.js" line: 915}]

Comment 9

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

Attachment: WIP - clean-up of the CameraControl API, v7

Preview starts and connects (though it's the green garbled mess from bug 880780)!

Comment 10

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

Attachment: WIP - clean-up of the CameraControl API, v8

Comment 11

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

Attachment: WIP - clean-up of the CameraControl API, v8.1

Comment 12

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

Attachment: WIP - clean-up of the CameraControl API, v9

Latest version -- likely not much has changed as there hasn't been a reliable platform to test on for a while.

Comment 13

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

Attachment: WIP - clean-up of the CameraControl API, v9.1

Merged in latest m-c changes, now up to 145908:b818c7222968

Comment 14

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

Attachment: WIP - clean-up of the CameraControl API, v10

Latest unbitrotted version of the patch, includes mods to apply cleanly to bug 807058 changes. Builds, not yet tested.

Comment 15

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

Attachment: WIP - clean-up of the CameraControl API, v11

This builds; taking pictures and recording video both work.

There is a long lag from tapping on the mode-switch button to actually changing modes; not sure where that is coming from--could be the result of a busy DEBUG build.

Comment 16

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

Attachment: WIP - clean-up of the CameraControl API, v12

This version adds WebRTC support back in, using the new native-layer API.

This builds but is untested.

Comment 17

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

Attachment: WIP - clean-up of the CameraControl API, v12.1

Remember to remove the CameraControlListener.

Comment 18

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

Attachment: WIP - clean-up of the CameraControl API, v13

Latest version includes updated, locally-passing test_camera.html!

try-server push:
https://tbpl.mozilla.org/?tree=Try&rev=1745248488a7

Comment 19

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

Comment on attachment 832940 [details] [diff] [review]
WIP - clean-up of the CameraControl API, v13

diego, djf: would you mind taking a look at the *.webidl and *.idl files in this patch, to make sure I haven't changed anything that makes your lives horrible?

The gist of the change is that instead of calling getCamera() and then getPreview() on the camera, you now just call getCamera(), which returns an object that is both a CameraControl and a MediaStream--so it can be connected directly to viewfinder.mozSrcOject. Not only does this greatly simplify the implementation (mine and yours, I believe), but it also means getCamera() can start the preview in parallel with the Camera app doing the rest of its post-get initialization.

As a result, I've removed the getPreviewStream/VideoMode() functions, and replaced them with a setMode() function that takes an argument of either 'picture' or 'video', along with an optional parameter that specifies preview size and/or recorder profile.

Currently, takePicture() still takes its load of arguments, but under the hood I've implemented individual accessors for file format, GPS information, picture size, etc., so this is easy enough to change in a later patch.

startRecording() still takes a load of parameters too; I think we can discuss the best approach to simplifying this method when we look at takePicture(). These two notwithstanding, please let me know what you think. I'd like to get reviews on these changes started this week. Thanks!

Comment 20

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

diego, djf: also, I've modified the platform code to automatically call resumePreview() internally, so that the app no longer has to. This makes the Camera app feel very snappy.

Comment 21

[David Flanagan [:djf]]

Comment on attachment 832940 [details] [diff] [review]
WIP - clean-up of the CameraControl API, v13

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

Mike,

I'm not actually very familiar with the current camera API, and so don't have much feedback to give on this patch.

Looking at the webidl, it appears that what you're changing is integreating the get preview stream methods with the getCamera() method.  I don't understand how the JS code will get the preview stream now, but I assume that I'm just missing something.

I find the name CameraOptions a little misleading because it is just a small subset of the available options.  I assume there is some distinction at a lower level between options that must be specified when the camera hardware is initialized and the other options that can be modified at any time.  It is fine with me to allow that distinction to show in the JS API.  But maybe choose your class names to make the distinction clear.  Maybe CameraOptions could be CameraConfiguration or CameraInitializationOptions or something?

Comment 22

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

David, thanks for the comments. The object returned by getCamera() is now _also_ the preview stream object. Before:

  gotStream(stream) { viewfinder.mozSrcObject = stream; }
  gotCamera(camera) { this._cameraObj = camera; camera.getPreviewStream(/* onSuccess = */gotStream); }
  init() { getCamera(/* onSuccess = */gotCamera); }

Now:

  gotCamera(camera) { this._cameraObj = camera; viewfinder.mozSrcObject = camera; }
  init() { getCamera(/* onSuccess = */gotCamera); }

As for 'CameraOptions', you're right: it's the smallest set of information CameraControl needs to get the camera up and running. I've been bikeshedding that name in my mind since this work began, and I think I like 'CameraConfiguration' better.

Comment 23

[Diego Marcos [:dmarcos]]

If getting access to the camera and accessing the stream are two operations triggered in parallel it wouldn't be more efficient to decouple them? What about providing two callbacks to the getCamera method?

getCamera(cameraNumber, gotCamera, gotStream)

gotCamera(camera) {...camera configuration...}
gotStream(stream) {viewfinder.mozSrcObject = stream}

This way the app can start configuring the app while the stream request is still pending. Also you make a clear distinction between the camera and the stream objects.

Comment 24

[Diego Marcos [:dmarcos]]

I'm not also sure if it's a good idea for the camera to keep it's configuration state. I think it would be cleaner if the gaia apps keep track of that state. What about passing an object to the takePicture method with the cameraConfiguration? e.g:

var cameraConfiguration = {
  thumbnailSize: {
    width: 640,
    height: 480
  },
  pictureSize : {
    width: 1280,
    height: 720
  },
  autoFocus: true,
  flash: "auto"
...
};

camera.takePicture(cameraConfiguration); // It'll take a picture with the configuration above

Comment 25

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

(In reply to Diego Marcos from comment #23)
>
> If getting access to the camera and accessing the stream are two operations
> triggered in parallel it wouldn't be more efficient to decouple them? What
> about providing two callbacks to the getCamera method?

Getting access to the camera object and the stream object are not parallel options. What's (now) done in parallel is starting the preview in hardware, something that in the old implementation didn't happen until the app had the camera object, then the stream object, and finally attached the latter to a <media> tag.

There's no need for the camera control and preview stream to be separate objects so I've combined them, which greatly simplifies the implementation; and since the preview needs to be started in hardware anyway for pictures to be taken or for recording to work (regardless of whether or not the frames are even sent to a <media> tag) I do that behind the scenes.

While that is going on in the Camera Thread (and in the kernel driver and camera hardware), the app can continue configuring itself on the Main Thread.

This should maximize the parallelization of the the camera app start up.

Comment 26

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

(In reply to Diego Marcos from comment #24)
>
> I'm not also sure if it's a good idea for the camera to keep it's
> configuration state. I think it would be cleaner if the gaia apps keep track
> of that state. What about passing an object to the takePicture method with
> the cameraConfiguration? e.g:

That's new--the last time we spoke about this, I thought we decided the camera would be responsible for keeping its configuration in Gecko.

> var cameraConfiguration = {
>   thumbnailSize: {
>     width: 640,
>     height: 480
>   },
>   pictureSize : {
>     width: 1280,
>     height: 720
>   },
>   autoFocus: true,
>   flash: "auto"
> ...
> };
> 
> camera.takePicture(cameraConfiguration); // It'll take a picture with the
> configuration above

This won't work: autoFocus needs to be set well in advance of taking the photo. Ditto for other settings that will affect what the viewfinder looks like.

My understanding from the last time we spoke was to keep .takePicture() minimal--just the callbacks--and to have the other settings as attributes set separately.

Comment 27

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

Attachment: WIP - clean-up of the CameraControl API, v14

Based on feedback, change setMode() and CameraOptions to setConfiguration() and CameraConfiguration. Also:
- make getCameras() and setConfiguration() take the same CameraConfiguration object
- add a new test for getCamera() providing default parameters, no call to setConfiguration()
- make sure maxFocusAreas and maxMeteringAreas are >= 0

Comment 28

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

Attachment: WIP - clean-up of the CameraControl API, v14.1

ACTUALLY add the new mochitest.

Comment 29

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

try-server push: https://tbpl.mozilla.org/?tree=Try&rev=1a9890ab78c9

Comment 30

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

Attachment: Part 1 - Gonk Implementation, v1

Comment 31

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

Attachment: Part 2 - DOM, v1

Comment 32

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

Attachment: Part 3 - WebRTC, v1

Comment 33

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

Attachment: Part 4 - Test, v1

Comment 34

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8334199 [details] [diff] [review]
Part 3 - WebRTC, v1

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

::: content/media/webrtc/MediaEngineWebRTCVideo.cpp
@@ +655,3 @@
>    ReentrantMonitorAutoEnter sync(mCallbackMonitor);
> +  switch (aState) {
> +    case CameraControlListener::kHardwareOpen:

do we expect other cases, or just this and default?  Are there any which would be nonsensical/surprising?  If no to both, maybe this should be an if - but if there are a bunch of states that fall into default, then switch is ok as it clues the reader into there being multiple options.

@@ +672,1 @@
>    ReentrantMonitorAutoEnter sync(mCallbackMonitor);

You're grabbing a monitor here - what thread(s) do these happen on?  The old HandleEvents all asserted mainthread.  What threads are waiting on these Notifys?

@@ +681,1 @@
>    mCallbackMonitor.Notify();

Are we throwing away aError on purpose?  (It looks like we threw it away before too)

::: content/media/webrtc/MediaEngineWebRTC.h
@@ +319,5 @@
>  class MediaEngineWebRTC : public MediaEngine
>  {
>  public:
>  #ifdef MOZ_B2G_CAMERA
> +  MediaEngineWebRTC()

This can be merged with the #else (move just the initializations inside the #ifdef)

Comment 35

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

(In reply to Randell Jesup [:jesup] from comment #34)
> Comment on attachment 8334199 [details] [diff] [review]
> Part 3 - WebRTC, v1
> 
> Review of attachment 8334199 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/media/webrtc/MediaEngineWebRTCVideo.cpp
> @@ +655,3 @@
> >    ReentrantMonitorAutoEnter sync(mCallbackMonitor);
> > +  switch (aState) {
> > +    case CameraControlListener::kHardwareOpen:
> 
> do we expect other cases, or just this and default?  Are there any which
> would be nonsensical/surprising?  If no to both, maybe this should be an if
> - but if there are a bunch of states that fall into default, then switch is
> ok as it clues the reader into there being multiple options.

Once upon a time there were more cases, but now it's just open and closed.
I'll change this to an if.

> @@ +672,1 @@
> >    ReentrantMonitorAutoEnter sync(mCallbackMonitor);
> 
> You're grabbing a monitor here - what thread(s) do these happen on?  The old
> HandleEvents all asserted mainthread.  What threads are waiting on these
> Notifys?

Camera OnEvent callbacks generally happen off the Main Thread. Some happen on
the Camera Thread, some are invoked from threads internal to the camera
library.

The monitor advanced the state here:
http://mxr.mozilla.org/mozilla-central/source/content/media/webrtc/MediaEngineWebRTCVideo.cpp#338

> @@ +681,1 @@
> >    mCallbackMonitor.Notify();
> 
> Are we throwing away aError on purpose?  (It looks like we threw it away
> before too)

The async camera operations either trigger an On*() callback, or OnError() if an error
occurred. The former callbacks all change mState and call .Notify() on the monitor; the
latter doesn't change mState, which the other waiting thread interprets as an error. So
'aError' is not required.

> ::: content/media/webrtc/MediaEngineWebRTC.h
> @@ +319,5 @@
> >  class MediaEngineWebRTC : public MediaEngine
> >  {
> >  public:
> >  #ifdef MOZ_B2G_CAMERA
> > +  MediaEngineWebRTC()
> 
> This can be merged with the #else (move just the initializations inside the
> #ifdef)

Would it be better to remove the #ifdef case here, leave only the declaration, and change MediaEngineWebRTC.cpp to have:
    ...
  mAudioEngineInit(false),
#ifdef MOZ_B2G_CAMERA
  mCameraManager(nullptr),
#endif
  mHasTabVideoSource(false)
    ...
?

Comment 36

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

rjesup, sorry I meant: remove the #ifdef case for the MediaEngineWebRTC ctor, leaving just the declaration, and then just #ifdef the AsyncLatencyLogger call into the ctor def'n in MediaEngineWebRTC.cpp.

Comment 37

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

Attachment: WIP - clean-up of the CameraControl API, v15 [m-c/b2g-inbound]

Rebased to m-c / b2g-inbound.

This incorporates the WebRTC review comments from rjesup.

try-server push: https://tbpl.mozilla.org/?tree=Try&rev=ef860e831e72

Comment 38

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

Attachment: Part 1 - Gonk Implementation, v2

Comment 39

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

Attachment: Part 2 - DOM, v2

Comment 40

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

Attachment: Part 3 - WebRTC, v2

Carrying r=jesup forward on this portion.

Comment 41

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

Attachment: Part 4 - Test, v2

Comment 42

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

New try-server push: https://tbpl.mozilla.org/?tree=Try&rev=2db93aed2902

(Breakage due to a missing 'AutoRwLock.h' in my b2g-inbound tree.)

Comment 43

[Randell Jesup [:jesup] (needinfo me)]

Looks good, thanks

Comment 44

[Dave Hylands [:dhylands]]

Comment on attachment 8334951 [details] [diff] [review]
Part 1 - Gonk Implementation, v2

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

Almost all of my comments were minor or nits.

The only real issue I noticed was the calculation of orientation.

r+ with these addressed.

::: dom/camera/CameraControlImpl.cpp
@@ +28,5 @@
> +  // reuse the same camera thread to conserve resources
> +  if (!sCameraThread) {
> +    nsresult rv = NS_NewNamedThread("Camera Thread", getter_AddRefs(sCameraThread));
> +    (void)rv; // swallow rv to suppress a compiler warning when the macro
> +              // is #defined to nothing (i.e. in non-DEBUG builds).

Use unused << rv; rather than a void cast. #include "mozilla/unused.h"

@@ +155,5 @@
>    }
>  }
>  
>  void
> +CameraControlImpl::OnRecorderStateChange(CameraControlListener::RecorderState aState, int32_t aStatus, int32_t aTrackNumber)

nit: line too long (> 80 chars)

::: dom/camera/CameraCommon.h
@@ +25,5 @@
>  #ifdef PR_LOGGING
>  extern PRLogModuleInfo* GetCameraLog();
>  #define DOM_CAMERA_LOG( type, ... ) PR_LOG(GetCameraLog(), (PRLogModuleLevel)type, ( __VA_ARGS__ ))
>  #else
> +#define DOM_CAMERA_LOG( type, ... ) printf_stderr(__VA_ARGS__) /* MFH */

So is this a remove before delivering?

::: dom/camera/GonkCameraControl.h
@@ +57,5 @@
> +  void OnTakePictureError();
> +  void OnNewPreviewFrame(layers::GraphicBufferLocked* aBuffer);
> +  void OnRecorderEvent(int msg, int ext1, int ext2);
> +
> +  nsresult Set(uint32_t aKey, const nsAString& aValue);

Same comment made elsewhere about virtual methods.

::: dom/camera/GonkCameraControl.cpp
@@ +156,5 @@
>    , mRecorder(nullptr)
>    , mProfileManager(nullptr)
>    , mRecorderProfile(nullptr)
>    , mVideoFile(nullptr)
> +  , mFileFormat()

not strictly necessary since the default constructor for a member will be called anyways.

@@ +206,5 @@
> +     *
> +     * The intent of the above flow is to let the Main Thread do as much work
> +     * up-front as possible without waiting for blocking Camera Thread calls
> +     * to complete.
> +     */

nice comment

@@ +328,5 @@
> +
> +    default:
> +      MOZ_ASSUME_UNREACHABLE("Unanticipated camera mode in SetConfigurationInternal()");
> +  }
> + 

nit: trailing space

@@ +376,5 @@
> +nsGonkCameraControl::SetVideoConfiguration(const CameraConfiguration& aConfig)
> +{
> +  nsresult rv = SetupVideoMode(aConfig.mRecorderProfile);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  

nit: trailing space

@@ +424,5 @@
> +nsGonkCameraControl::EndBatchParameterSet()
> +{
> +  RwLockAutoEnterWrite lock(mRwLock);
> +  --mDeferConfigUpdate;
> +  MOZ_ASSERT(mDeferConfigUpdate != UINT32_MAX); // catch underflow badness

I'd probably assert (mDeferConfigUpdate > 0) before the increment. Then you don't have to rely on the underflow behaviour.

@@ +449,5 @@
> +nsGonkCameraControl::Get(const char* aKey, const char** aRet)
> +{
> +  NS_ENSURE_TRUE(aRet, NS_ERROR_INVALID_ARG);
> +  RwLockAutoEnterRead lock(mRwLock);
> +  *aRet = mParams.get(aKey);

So it looks like *aRet has the same lifetime as mParams or until the same paramter is set to a different value.

Are the callers that use this making a copy? Or only use the pointer for the same lifetime as mParams?

@@ +485,5 @@
> +  if (!value) {
> +    aRet = NS_LITERAL_STRING("");
> +    return NS_OK;
> +  }
> + 

nit: trailing space

@@ +504,5 @@
> +      /**
> +       * Convert from real value to a Gonk index, round
> +       * to the nearest step; index is 1-based.
> +       */
> +      uint32_t index = (aValue - mExposureCompensationMin + mExposureCompensationStep / 2) / mExposureCompensationStep + 1;

nit: line greater than 80 (I've been called on that a few time) this seemed like a blatantly obvious case...

@@ +600,5 @@
> +      r += mCameraHw->GetSensorOrientation();
> +      r %= 360;
> +      r += 45;
> +      r /= 90;
> +      r *= 90;

Shouldn't this do the % 360 at the end?

Otherwise, if r= 359 when you start

r %= 360 gives 359
r += 45 gives 404
r /= 90 gives 4
r *= 90 gives 360

Don't you want 0 for this case?

@@ +958,3 @@
>  
> +  DOM_CAMERA_LOGI("stopping preview (this=%p)\n", this);
> + 

nit: trailing space

@@ +1084,5 @@
>     */
>    int smallestDelta = INT_MAX;
>    uint32_t smallestDeltaIndex = UINT32_MAX;
>    int targetArea = aWidth * aHeight;
>    

nit: trailing space (this one wasn't part of your patch)

@@ +1135,4 @@
>    if (mCameraHw->TakePicture() != OK) {
>      return NS_ERROR_FAILURE;
>    }
>    

nit: trailing space (not part of your patch)

@@ +1738,5 @@
>  
> +void
> +OnError(nsGonkCameraControl* gc, int32_t aArg1, int32_t aArg2)
> +{
> +  // swallow 'aArg1' and 'aArg2' for now.

#include "mozilla/unused.h"

unused << aArg1;
unused << aArg2;

will get rid of warnings about unused vars.

::: dom/camera/GonkCameraSource.cpp
@@ +170,5 @@
>        mNumFramesDropped(0),
>        mNumGlitches(0),
>        mGlitchDurationThresholdUs(200000),
> +      mCollectStats(false),
> +      mCameraHw(aCameraHw) {

In the other files, you used the comma at the beginning of the line for member initializers, here you're doing it at the end.

::: dom/camera/GonkCameraManager.cpp
@@ +104,5 @@
>  
>    if (!gotFront) {
>      aList.RemoveElementAt(1);
>    }
>    

nit: trailing space (outside of your patch)

::: dom/camera/GonkCameraHwMgr.cpp
@@ +109,5 @@
>      return;
>    }
>  
>    switch (aMsgType) {
>      case CAMERA_MSG_FOCUS:

success only seems to be used in this case. Declare success inside the case rather than at the function level. You may need to introduce braces.

::: dom/camera/CameraControlImpl.h
@@ +11,3 @@
>  #include "nsProxyRelease.h"
>  #include "DictionaryHelpers.h"
> +#include "AutoRwLock.h"

Where does this come from?

I couldn't find it in our tree, and does it get used in this file?

@@ +28,5 @@
>  class CameraControlImpl : public ICameraControl
>  {
> +public:
> +  CameraControlImpl(uint32_t aCameraId);
> +  void AddListener(CameraControlListener* aListener);

It looks like this is overriding a virtual member in the base class. I normally like to declate these virtual here, which makes it a little more obvious that they're virtual.

The Coding Style Guide says:
    Method declarations that override virtual methods from a base class should be annotated with both "virtual" and "MOZ_OVERRIDE".

@@ +63,5 @@
> +  void OnPreviewStateChange(CameraControlListener::PreviewState aState);
> +  void OnHardwareStateChange(CameraControlListener::HardwareState aState);
> +  void OnConfigurationChange();
> +
> +  static nsRefPtr<nsIThread> sCameraThread;

I think that this should be nsCOMPtr (use nsCOMPtr for nsI stuff and nsRefPtr for concrete classes)

@@ +64,5 @@
> +  void OnHardwareStateChange(CameraControlListener::HardwareState aState);
> +  void OnConfigurationChange();
> +
> +  static nsRefPtr<nsIThread> sCameraThread;
> +  nsIThread* CameraThread() { return sCameraThread; }

I think that this should return:

already_AddRefed<nsIThread>

so that the caller isn't tempted to assign it to a bare pointer.

Comment 45

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

Thanks for the review! Specific responses below. (If I don't respond to something, it's because I agree and have accepted the suggestion.)

(In reply to Dave Hylands [:dhylands] from comment #44)
> Comment on attachment 8334951 [details] [diff] [review]
> Part 1 - Gonk Implementation, v2
> 
> Review of attachment 8334951 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Almost all of my comments were minor or nits.
> 
> The only real issue I noticed was the calculation of orientation.
> 
> r+ with these addressed.
> 
> ::: dom/camera/CameraCommon.h
> @@ +25,5 @@
> >  #ifdef PR_LOGGING
> >  extern PRLogModuleInfo* GetCameraLog();
> >  #define DOM_CAMERA_LOG( type, ... ) PR_LOG(GetCameraLog(), (PRLogModuleLevel)type, ( __VA_ARGS__ ))
> >  #else
> > +#define DOM_CAMERA_LOG( type, ... ) printf_stderr(__VA_ARGS__) /* MFH */
> 
> So is this a remove before delivering?

Yes. I'll take that out last.

> @@ +206,5 @@
> > +     *
> > +     * The intent of the above flow is to let the Main Thread do as much work
> > +     * up-front as possible without waiting for blocking Camera Thread calls
> > +     * to complete.
> > +     */
> 
> nice comment

Thanks. :)

> @@ +449,5 @@
> > +nsGonkCameraControl::Get(const char* aKey, const char** aRet)
> > +{
> > +  NS_ENSURE_TRUE(aRet, NS_ERROR_INVALID_ARG);
> > +  RwLockAutoEnterRead lock(mRwLock);
> > +  *aRet = mParams.get(aKey);
> 
> So it looks like *aRet has the same lifetime as mParams or until the same
> paramter is set to a different value.
> 
> Are the callers that use this making a copy? Or only use the pointer for the
> same lifetime as mParams?

No, this is a messy bug that needs fixing. (I'm working on a revamp of the configuration management in bug 936111, but I should probably fix this here and now.)

> ::: dom/camera/GonkCameraSource.cpp
> @@ +170,5 @@
> >        mNumFramesDropped(0),
> >        mNumGlitches(0),
> >        mGlitchDurationThresholdUs(200000),
> > +      mCollectStats(false),
> > +      mCameraHw(aCameraHw) {
> 
> In the other files, you used the comma at the beginning of the line for
> member initializers, here you're doing it at the end.

This file is based on a AOSP file. The style differences are due to the original non-Mozilla style.

> ::: dom/camera/CameraControlImpl.h
> @@ +11,3 @@
> >  #include "nsProxyRelease.h"
> >  #include "DictionaryHelpers.h"
> > +#include "AutoRwLock.h"
> 
> Where does this come from?
> 
> I couldn't find it in our tree, and does it get used in this file?

Yes, I forgot to 'hg add' it to my repo when I exported the patch for review. It's basically an RAII wrapper for a PRRWLock. It will be in the updated patch.

I'm going to rejig handling the CameraThread lifetime, and fix the param value lifetimes bug. I'll r? you on the updated patch.

Comment 46

[[e-mail] [:onecyrenus]]

Comment on attachment 8334956 [details] [diff] [review]
Part 4 - Test, v2

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

Would say if we add more camera tests, it is probably a good idea to refactor so the Camera object is a library.

Comment 47

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

Attachment: WIP - clean-up of the CameraControl API, v16 [m-c/b2g-inbound]

This latest version includes all review feedback and migrates the entirety of the API to WebIDL. No more XPIDL. It also turns off automatic preview restarting, due to bug 950102.

It contains a crashbug (on hamachi) that I am currently diagnosing. Will update the Part X patches once it is resolved.

Comment 48

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

Attachment: WIP - clean-up of the CameraControl API, v16.1 [m-c/b2g-inbound]

Stable on hamachi.

Comment 49

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

Attachment: WIP - clean-up of the CameraControl API, v17 [m-c/b2g-inbound]

try-server push: https://tbpl.mozilla.org/?tree=Try&rev=8dd5b7cf4a53

Comment 50

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

Attachment: Part 1 - Gonk Implementation, v3

Incorporates feedback from previous review; also:
- removes all DOM-things from the ICameraControl interface and implementation
- wraps all camera parameters in a threadsafe class

Comment 51

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

Attachment: Part 1 - Gonk Implementation, v3.1

Attach the correct patch this time!

Comment 52

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

Attachment: Part 2 - DOM, v3

jst, as requested: this patch removes all XPIDL-isms from the DOM interface. The camera interface is now 100% WebIDL.

Comment 53

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

Attachment: Part 3 - WebRTC, v3, r=jesup

Carrying r+ forward; changes from previous version are due to ICameraControl interface changes.

Comment 54

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

Attachment: Part 4 - Test, v3, r=onecyrenus

Carrying r+ forward.

Comment 56

[Johnny Stenback (:jst)]

Comment on attachment 8349634 [details] [diff] [review]
Part 2 - DOM, v3

Mike, thanks a *ton* for working through all this stuff and getting this stuff to use the WebIDL bindings! This stuff looks great, still room for some followups etc here (some of which you've already filed, even!), but I love this so far. r=jst, with the few mostly minor things below looked into.

- In dom/camera/DOMCameraCapabilities.cpp:

+CameraCapabilities::~CameraCapabilities()
 {
...
+  mRecorderProfiles = JS::UndefinedValue();

This *should* not be neded AFAIK.

- In CameraCapabilities::Populate(ICameraControl* aCameraControl):

+  // For now, always return success, since the presence or absence of capabilities
+  //  indicates whether or not they are supported.

Remove the extra space there?

+// JS-to-native helpers
+// Setter for weighted regions: { top, bottom, left, right, weight }
+nsresult
+nsDOMCameraControl::Set(JSContext* aCx, uint32_t aKey, const JS::Value& aValue, uint32_t aLimit)

Seem this, and the ::Get() method as well, could be removed in a followup bug in favor of more WebIDL dictionaries, right?

- In nsDOMCameraControl::OnError():

+  nsCOMPtr<CameraErrorCallback>* errorCb;
+  switch (aContext) {
...
+    default:
+      MOZ_ASSUME_UNREACHABLE("Error occurred in unanticipated camera state");
+  }
+
+  MOZ_ASSERT(errorCb);

If we fall into the default case in the switch, then errorCb will be uninitialized here...

+  if (!*errorCb) {

... and this will read arbitrary memory.

+  ErrorResult ignored;
+  (*errorCb)->Call(aError, ignored);

Since errorCb is a pointer to an nsCOMPtr, I wonder if we need a kunfFuDeathGrip here. Is there a risk of the called callback being able to reach back into this object and unset the member that currently holds the callback alive? If so, we need to make sure something holds this alive...

Happy Hoildays! :)

Comment 57

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

Thanks for the review!

(In reply to Johnny Stenback (:jst, [e-mail]) from comment #56)
> Comment on attachment 8349634 [details] [diff] [review]
> Part 2 - DOM, v3
> 
> Mike, thanks a *ton* for working through all this stuff and getting this
> stuff to use the WebIDL bindings!

Not a problem. The upfront conversion work was a pain, but now that it's done,
WebIDL will definitely be easier to maintain and extend.

> This stuff looks great, still room for
> some followups etc here (some of which you've already filed, even!), but I
> love this so far. r=jst, with the few mostly minor things below looked into.
> 
> - In dom/camera/DOMCameraCapabilities.cpp:
> 
> +CameraCapabilities::~CameraCapabilities()
>  {
> ...
> +  mRecorderProfiles = JS::UndefinedValue();
> 
> This *should* not be neded AFAIK.

You caught me: I was largely cargo-culting the native Promise implementation here:
http://mxr.mozilla.org/mozilla-central/source/dom/promise/Promise.cpp#191

> +// JS-to-native helpers
> +// Setter for weighted regions: { top, bottom, left, right, weight }
> +nsresult
> +nsDOMCameraControl::Set(JSContext* aCx, uint32_t aKey, const JS::Value&
> aValue, uint32_t aLimit)
> 
> Seem this, and the ::Get() method as well, could be removed in a followup
> bug in favor of more WebIDL dictionaries, right?

I initially tried that, and ran into this error:

WebIDL.WebIDLError: error: An attribute cannot be of a dictionary type, /home/mikeh/dev/mozilla/m-c/b2g-inbound/dom/webidl/CameraControl.webidl line 193:2
  attribute CameraRegion             meteringAreas;
  ^

> - In nsDOMCameraControl::OnError():
> 
> +  nsCOMPtr<CameraErrorCallback>* errorCb;
> +  switch (aContext) {
> ...
> +    default:
> +      MOZ_ASSUME_UNREACHABLE("Error occurred in unanticipated camera
> state");
> +  }
> +
> +  MOZ_ASSERT(errorCb);
> 
> If we fall into the default case in the switch, then errorCb will be
> uninitialized here...
>
> +  if (!*errorCb) {
> 
> ... and this will read arbitrary memory.

MOZ_ASSUME_UNREACHABLE() calls MOZ_CRASH(), but an extra "return;" in there would
definitely make it very obvious what's going on.

> +  ErrorResult ignored;
> +  (*errorCb)->Call(aError, ignored);
> 
> Since errorCb is a pointer to an nsCOMPtr, I wonder if we need a
> kunfFuDeathGrip here. Is there a risk of the called callback being able to
> reach back into this object and unset the member that currently holds the
> callback alive? If so, we need to make sure something holds this alive...

Good point--that should probably apply to all of the other callbacks as well.
I'll fix them all.

Comment 58

[Dave Hylands [:dhylands]]

Comment on attachment 8349633 [details] [diff] [review]
Part 1 - Gonk Implementation, v3.1

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

Looks good to me.

Most of my comments were minor. But I have some concerns about thread safety in nsGonkCameraControl that I'd like to see addressed.

::: dom/camera/CameraControlImpl.cpp
@@ +278,5 @@
> +
> +    nsresult rv = RunImpl();
> +    if (NS_FAILED(rv)) {
> +      DOM_CAMERA_LOGW("Camera control API failed at %d with %d\n", mContext, rv);
> +      // XXXmikeh - do we want to report a more specific error code?

XXXmikeh?

@@ +553,5 @@
> +  RwLockAutoEnterWrite lock(mListenerLock);
> +
> +  nsRefPtr<CameraControlListener> l(aListener);
> +  mListeners.RemoveElement(l);
> +  // XXXmikeh - do we want to notify the listener that it has been removed?

XXXmikeh?

::: dom/camera/GonkCameraParameters.h
@@ +27,5 @@
> +namespace mozilla {
> +
> +class GonkCameraParameters
> +{
> +protected:

nit: I think that the general preference is to put all of the public methods first, followed by protected and then finally private

::: dom/camera/CameraCommon.h
@@ +24,5 @@
>  #ifdef PR_LOGGING
>  extern PRLogModuleInfo* GetCameraLog();
>  #define DOM_CAMERA_LOG( type, ... ) PR_LOG(GetCameraLog(), (PRLogModuleLevel)type, ( __VA_ARGS__ ))
>  #else
> +#define DOM_CAMERA_LOG( type, ... ) printf_stderr(__VA_ARGS__) /* MFH */

I see the MFH. Does this mean you meant to remove this?

::: dom/camera/GonkCameraControl.cpp
@@ +249,2 @@
>    }
> +  

nit: trailing spaces

@@ +330,4 @@
>      return mCameraThread->Dispatch(pushParametersTask, NS_DISPATCH_NORMAL);
>    }
>  
>    DOM_CAMERA_LOGT("%s:%d\n", __func__, __LINE__);

Just a comment: Why not make DOM_CAMERA_LOGT record __func__ and __LINE__ everytime its called?

@@ +339,2 @@
>  {
> +  uint32_t dcu = ++mDeferConfigUpdate;

Can this function run on more than one thread? If so, then this increment (and the corresponding decrement) may be problematic.

If not, lets assert that we're on the right thread.

@@ +573,5 @@
>     * We keep a copy of the specified size so that if the picture size
>     * changes, we can choose a new thumbnail size close to what was asked for
>     * last time.
>     */
> +  mLastThumbnailSize = aSize;

Can functions like this get called from multiple threads?

It seems like a bunch of these functions are thread safe, and some are not. I'd really like to see the ones that are only called from one thread assert that they're on the right thread.

@@ +1004,2 @@
>  
> +    // XXXmikeh: the camera interface allows for hardware to provide two video

xxxmikeh?

::: dom/camera/AutoRwLock.h
@@ +12,5 @@
> +public:
> +  RwLockAutoEnterRead(PRRWLock* aRwLock)
> +    : mRwLock(aRwLock)
> +  {
> +    PR_RWLock_Rlock(mRwLock);

nit: ASSERT that aRwLock is non-null

@@ +30,5 @@
> +public:
> +  RwLockAutoEnterWrite(PRRWLock* aRwLock)
> +    : mRwLock(aRwLock)
> +  {
> +    PR_RWLock_Wlock(mRwLock);

nit: ASEERT non-null

::: dom/camera/ICameraControl.h
@@ +18,3 @@
>  class RecorderProfileManager;
>  
> +// XXXmikeh - In some future patch this should move into the ICameraControl

XXXmikeh?

::: dom/camera/GonkCameraParameters.cpp
@@ +15,5 @@
> + */
> +
> +#include "camera/CameraParameters.h"
> +#include "ICameraControl.h"
> +#include "GonkCameraParameters.h"

nit: SomeFile.cpp should #include SomeFile.h as the very first #include (that verifies that SomeFile.h includes all of the dependent headers)

@@ +145,5 @@
> +{
> +  nsresult rv;
> +
> +  rv = GetImpl(CAMERA_PARAM_SUPPORTED_MINEXPOSURECOMPENSATION, mExposureCompensationMin);
> +  NS_ENSURE_SUCCESS(rv, rv);

nit: I read that we're not supposed to be using these macros anymore.

https://groups.google.com/forum/#!topic/mozilla.dev.platform/1clMLuuhtWQ

@@ +187,5 @@
> +  switch (aKey) {
> +    case CAMERA_PARAM_THUMBNAILSIZE:
> +      // This is a special case--for some reason the thumbnail size
> +      // is accessed as two separate values instead of a tuple.
> +      // XXXmikeh - make this restore the original values on error

XXXmikeh?

@@ +293,5 @@
> +  // count the number of regions in the string
> +  while ((p = strstr(p, "),("))) {
> +    ++count;
> +    p += 3;
> +  }

Is zero regions valid?

This code will treat an empty string as 1 region.

Perhaps just counting the number of '(' or ')' would be better?

I think this loop should use the same criteria as the loop below (which is using strchr on '(')

@@ +387,5 @@
> +
> +    default:
> +      {
> +        // You can't actually pass 64-bit parameters to Gonk. :(
> +        int32_t v = aValue;

Shouldn't this line also have the static_cast ? to get rid of the compiler warning?

@@ +440,5 @@
> +  switch (aKey) {
> +    case CAMERA_PARAM_ZOOM:
> +      rv = GetImpl(CAMERA_PARAM_ZOOM, val);
> +      if (NS_SUCCEEDED(rv)) {
> +        val /= 100;

nit: use 100.0

@@ +580,5 @@
> +    return NS_ERROR_NOT_AVAILABLE;
> +  }
> +
> +  aArray.Clear();
> +  const char* q = p;

nit: Can we call q something a little more meaningful like comma?

Also I don't see why you'd be assigning q to be p since you assign q to be strchr(p, ',') before you use q.

::: dom/camera/GonkCameraHwMgr.cpp
@@ +69,5 @@
>        break;
>  
>      case CAMERA_MSG_COMPRESSED_IMAGE:
>        if (aDataPtr != nullptr) {
> +        OnTakePictureComplete(mTarget, (uint8_t*)aDataPtr->pointer(), aDataPtr->size());

nit: use static_cast rather than C-style cast

::: dom/camera/CameraControlImpl.h
@@ +63,5 @@
> +  void OnHardwareStateChange(CameraControlListener::HardwareState aState);
> +  void OnConfigurationChange();
> +
> +  static nsWeakPtr sCameraThread;
> +  nsCOMPtr<nsIThread> mCameraThread;

Perhaps a comment explaining why you have both a static and a member?

Comment 59

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

(In reply to Dave Hylands [:dhylands] from comment #58)
> Comment on attachment 8349633 [details] [diff] [review]
> Part 1 - Gonk Implementation, v3.1
> 
> Review of attachment 8349633 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good to me.
> 
> Most of my comments were minor. But I have some concerns about thread safety
> in nsGonkCameraControl that I'd like to see addressed.
> 
> ::: dom/camera/CameraControlImpl.cpp
> @@ +278,5 @@
> > +
> > +    nsresult rv = RunImpl();
> > +    if (NS_FAILED(rv)) {
> > +      DOM_CAMERA_LOGW("Camera control API failed at %d with %d\n", mContext, rv);
> > +      // XXXmikeh - do we want to report a more specific error code?
> 
> XXXmikeh?

This seems to be the style for making notes on things we may want to revisit. I'm open to better ideas, but grep shows XXX[userid] all over the place.

> ::: dom/camera/CameraCommon.h
> @@ +24,5 @@
> >  #ifdef PR_LOGGING
> >  extern PRLogModuleInfo* GetCameraLog();
> >  #define DOM_CAMERA_LOG( type, ... ) PR_LOG(GetCameraLog(), (PRLogModuleLevel)type, ( __VA_ARGS__ ))
> >  #else
> > +#define DOM_CAMERA_LOG( type, ... ) printf_stderr(__VA_ARGS__) /* MFH */
> 
> I see the MFH. Does this mean you meant to remove this?

Yes, this will be removed before posting the final submittable patch.

> @@ +330,4 @@
> >      return mCameraThread->Dispatch(pushParametersTask, NS_DISPATCH_NORMAL);
> >    }
> >  
> >    DOM_CAMERA_LOGT("%s:%d\n", __func__, __LINE__);
> 
> Just a comment: Why not make DOM_CAMERA_LOGT record __func__ and __LINE__
> everytime its called?

That's on my to-do list. (I tried this once before and ran into some compiler-specific behaviour that I didn't feel like wrangling at the time.)

> @@ +339,2 @@
> >  {
> > +  uint32_t dcu = ++mDeferConfigUpdate;
> 
> Can this function run on more than one thread? If so, then this increment
> (and the corresponding decrement) may be problematic.
> 
> If not, lets assert that we're on the right thread.

It can, which is why mDeferConfigUpdate is Atomic<uint32_t> in GonkCameraControl.h.

> @@ +573,5 @@
> >     * We keep a copy of the specified size so that if the picture size
> >     * changes, we can choose a new thumbnail size close to what was asked for
> >     * last time.
> >     */
> > +  mLastThumbnailSize = aSize;
> 
> Can functions like this get called from multiple threads?
> 
> It seems like a bunch of these functions are thread safe, and some are not.
> I'd really like to see the ones that are only called from one thread assert
> that they're on the right thread.

These are usually only called from the CameraThread or the Main Thread, but now that I look at it, it makes me uneasy. I'll address this before submitting. Thanks.

> @@ +145,5 @@
> > +{
> > +  nsresult rv;
> > +
> > +  rv = GetImpl(CAMERA_PARAM_SUPPORTED_MINEXPOSURECOMPENSATION, mExposureCompensationMin);
> > +  NS_ENSURE_SUCCESS(rv, rv);
> 
> nit: I read that we're not supposed to be using these macros anymore.
> 
> https://groups.google.com/forum/#!topic/mozilla.dev.platform/1clMLuuhtWQ

I'll fix this in the new file, but will save a wholesale removal of the deprecated macros for a future patch.

> @@ +293,5 @@
> > +  // count the number of regions in the string
> > +  while ((p = strstr(p, "),("))) {
> > +    ++count;
> > +    p += 3;
> > +  }
> 
> Is zero regions valid?
>
> This code will treat an empty string as 1 region.

Yes. In general I've found that the CameraParameters API can return all kinds of things: nullptr, "\0", in addition to valid values. I'll add a case here and in other places to catch empty strings as well and short-circuit-return in those cases.

Thanks for the thorough review!

Comment 60

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

Attachment: WIP - clean-up of the CameraControl API, v18 [m-c/b2g-inbound]

Incorporating feedback from dhylands' review.

try-server weirdness, trying to figure out what's going on:
https://tbpl.mozilla.org/?tree=Try&rev=729e8c8dde9a

Comment 61

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

Attachment: WIP - clean-up of the CameraControl API, v18.1 [m-c/b2g-inbound]

A version of the patch that isn't iNsAnE.

Comment 62

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

Attachment: WIP - clean-up of the CameraControl API, v18.2 [m-c/b2g-inbound]

Remove stale references to a no-longer existing XPIDL module from package info.

try-server push:
https://tbpl.mozilla.org/?tree=Try&rev=b7b7c57dab56

Comment 63

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

Attachment: WIP - clean-up of the CameraControl API, v19 [m-c/b2g-inbound]

Comment 64

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

try-server push: https://tbpl.mozilla.org/?tree=Try&rev=2a7dfb60ab88

Comment 65

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

Attachment: Clean-up of the CameraControl API, v20 [m-c/b2g-inbound]

Unbitrotted.

Comment 66

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

Attachment: Clean-up of the CameraControl API, v21 [m-c/b2g-inbound]

Unbitrot.

Comment 67

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

Attachment: Clean-up of the CameraControl API, v22 [m-c/b2g-inbound]

More unbitrotting.

Comment 68

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

try-server push: https://tbpl.mozilla.org/?tree=Try&rev=38f1f8a1bf18

Comment 69

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

Attachment: Clean-up of the CameraControl API, v23 [m-c/b2g-inbound]

More unbitrotting.

Comment 70

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

Attachment: Clean-up of the CameraControl API, v24 [b2g-inbound]

Unbitrotting breakage due to bug 910498 landing.

Comment 71

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

try-server push: https://tbpl.mozilla.org/?tree=Try&rev=99ea30894e76 (includes tests for bug 940424).

Comment 72

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

Attachment: Part 2 - DOM, v4

This updated version of the DOM-specific changes changes the nsIDOMEventListener subclassing of nsDOMCameraControl to a helper class.

Comment 73

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

dhylands: see DOMCameraControl.* in attachment 8363865 [details] [diff] [review].

Comment 74

[Dave Hylands [:dhylands]]

Comment on attachment 8363865 [details] [diff] [review]
Part 2 - DOM, v4

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

So I reviewed the StartRecording changes in DOMCameraControl.h/.cpp

r=me if you fix the memory leak.

::: dom/camera/DOMCameraControl.cpp
@@ +104,5 @@
> +    MOZ_COUNT_CTOR(StartRecordingHelper);
> +  }
> +  ~ StartRecordingHelper()
> +  {
> +    MOZ_COUNT_CTOR(StartRecordingHelper);

Shouldn't this be MOZ_COUNT_DTOR ?

@@ +177,5 @@
> +  SetIsDOMBinding();
> +
> +  nsRefPtr<DOMCameraConfiguration> initialConfig =
> +    new DOMCameraConfiguration(aInitialConfig);
> +  

nit:trailing space

@@ +297,5 @@
> +{
> +  nsTArray<ICameraControl::Region> regionArray;
> +
> +  nsresult rv = mCameraControl->Get(aKey, regionArray);
> +  NS_ENSURE_SUCCESS(rv, rv);

NS_ENSURE_SUCCESS has been deprecated.
See https://groups.google.com/forum/#!topic/mozilla.dev.platform/1clMLuuhtWQ

@@ +512,3 @@
>      return;
>    }
> +  

nit: trailing space

@@ +524,1 @@
>    

nit: trailing space

@@ +721,1 @@
>                                           getter_AddRefs(request));

nit: indentation

@@ +732,4 @@
>  
> +  nsCOMPtr<nsIDOMEventListener> listener = new StartRecordingHelper(this);
> +  request->AddEventListener(NS_LITERAL_STRING("success"), listener, false);
> +  request->AddEventListener(NS_LITERAL_STRING("error"), listener, false);

So where do these get removed? This seems like you're going to keep adding new listeners every time StartRecording is called.

@@ +831,5 @@
> +    ErrorResult ignored;
> +    ecb->Call(NS_LITERAL_STRING("Interrupted"), ignored);
> +    cancel = true;
> +  }
> +    

nit: trailing space

Comment 75

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

Attachment: Clean-up of the CameraControl API, v25 [b2g-inbound]

Latest unbitrotting.

Comment 76

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

(In reply to Dave Hylands [:dhylands] from comment #74)
> Comment on attachment 8363865 [details] [diff] [review]
> Part 2 - DOM, v4
> 
> Review of attachment 8363865 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> So I reviewed the StartRecording changes in DOMCameraControl.h/.cpp
> 
> r=me if you fix the memory leak.
> 
> ::: dom/camera/DOMCameraControl.cpp
> @@ +104,5 @@
> > +    MOZ_COUNT_CTOR(StartRecordingHelper);
> > +  }
> > +  ~ StartRecordingHelper()
> > +  {
> > +    MOZ_COUNT_CTOR(StartRecordingHelper);
> 
> Shouldn't this be MOZ_COUNT_DTOR ?

Good eye. :)

> @@ +732,4 @@
> >  
> > +  nsCOMPtr<nsIDOMEventListener> listener = new StartRecordingHelper(this);
> > +  request->AddEventListener(NS_LITERAL_STRING("success"), listener, false);
> > +  request->AddEventListener(NS_LITERAL_STRING("error"), listener, false);
> 
> So where do these get removed? This seems like you're going to keep adding
> new listeners every time StartRecording is called.

I've confirmed that when the DOMRequest object expires, the listeners are destroyed as well.

Comment 77

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

Attachment: Clean-up of the CameraControl API, v26 [b2g-inbound]

Rebased

Comment 78

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

Attachment: Clean-up of the CameraControl API, v27 [b2g-inbound]

Fix build failure when B2G_DEBUG=1

Comment 79

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

Attachment: Clean-up of the CameraControl API, v27.1 [b2g-inbound]

Fix broken Win32 and MacOSX builds.
try-server results: https://tbpl.mozilla.org/?tree=Try&rev=0eb436123ebc

Comment 80

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

Attachment: Clean-up of the CameraControl API, v27.2 [b2g-inbound]

Rebased.

Comment 81

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

Attachment: Clean-up of the CameraControl API, v27.3 [b2g-inbound]

Fix the takePicture() rotation bug.

Comment 82

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

Attachment: Clean-up of the CameraControl API, v27.4 [b2g-inbound] r=dhylands,jesup,jst,onecyrenus

Carrying r+ forward

Comment 83

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

Attachment: Clean-up of the CameraControl API, v27.5 [b2g-inbound] r=dhylands,jesup,jst,onecyrenus

Rebased.
try-server push: https://tbpl.mozilla.org/?tree=Try&rev=94e1a077a469

Comment 84

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

Attachment: Clean-up of the CameraControl API, v27.6 [b2g-inbound] r=dhylands,jesup,jst,onecyrenus

Make sure OnAutoFocusComplete() calls PullParametersImpl() on the Camera Thread, else Camera app will crash in DEBUG builds.

try-server push: https://tbpl.mozilla.org/?tree=Try&rev=860467930c7e

Comment 85

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

Comment on attachment 8372406 [details] [diff] [review]
Clean-up of the CameraControl API, v27.6 [b2g-inbound] r=dhylands,jesup,jst,onecyrenus

(Carrying earlier r+ forward.)

Comment 86

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

New try-server push with DEBUG builds enabled: https://tbpl.mozilla.org/?tree=Try&rev=ec1df9f56813

Comment 87

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

https://hg.mozilla.org/integration/b2g-inbound/rev/f03664765359

Comment 88

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/b2g-inbound/rev/f6db7c7f226f for https://tbpl.mozilla.org/php/getParsedLog.php?id=34455470&tree=B2g-Inbound - the good news is that you weren't actually getting the 34 assertions you expected in camera_2; the bad news is that you were shoving one assertion off into the next test, which is rarely a good sign.

Oh, and if you were trying to get debug mochitests to run in that try push in comment 86, well... we wear clownshoes an awful lot, and that would be one place: you can run a particular chunk on debug emulator builds if you know the magic unlisted name, mochitest-debug-3, but because there's a different number of chunks for opt and debug and thus tests run in different chunks on each, you almost certainly won't actually get the right chunk for both opt and debug.

Comment 89

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Comment on attachment 8372406 [details] [diff] [review]
Clean-up of the CameraControl API, v27.6 [b2g-inbound] r=dhylands,jesup,jst,onecyrenus

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

MikeH,
This patch is an amazing code refactoring. I want to understand the latest camera gecko codes. So I traced this patch and found some nits for your reference. Hope you don't mind.

::: content/media/webrtc/MediaEngineWebRTC.h
@@ +45,5 @@
>  #include "webrtc/video_engine/include/vie_codec.h"
>  #include "webrtc/video_engine/include/vie_render.h"
>  #include "webrtc/video_engine/include/vie_capture.h"
>  #ifdef MOZ_B2G_CAMERA
> +// #include "CameraPreviewMediaStream.h"

Should we keep this line?

::: dom/camera/CameraControlImpl.cpp
@@ +10,2 @@
>  #include "CameraRecorderProfiles.h"
>  #include "CameraControlImpl.h"

Redundant line. Already include CameraControlImpl.h in line 5.

@@ +152,2 @@
>  void
>  CameraControlImpl::OnClosed()

Can we just use |OnHardwareStateChange| and remove this function?

@@ +281,5 @@
> +
> +    nsresult rv = RunImpl();
> +    if (NS_FAILED(rv)) {
> +      DOM_CAMERA_LOGW("Camera control API failed at %d with 0x%x\n", mContext, rv);
> +      // XXXmikeh - do we want to report a more specific error code?

How about left the decision of showing specific error code to the functions be called by |RunImpl|? For example: |SetConfigurationImpl|, |AutoFocusImpl| and so on.

@@ +551,5 @@
> +  RwLockAutoEnterWrite lock(mListenerLock);
> +
> +  nsRefPtr<CameraControlListener> l(aListener);
> +  mListeners.RemoveElement(l);
> +  // XXXmikeh - do we want to notify the listener that it has been removed?

1. Can we allow the listener to be removed by others?
2. Can the owner of the listener be able to delete the pointer of the listener? I mean can we allow nsDOMCameraControl to delete his mListener?

::: dom/camera/DOMCameraControl.cpp
@@ +179,5 @@
> +  SetIsDOMBinding();
> +
> +  nsRefPtr<DOMCameraConfiguration> initialConfig =
> +    new DOMCameraConfiguration(aInitialConfig);
> +  

nit: leading space

@@ +493,5 @@
>  JS::Value
>  nsDOMCameraControl::GetPictureSize(JSContext* cx, ErrorResult& aRv)
>  {
>    JS::Rooted<JS::Value> value(cx);
>    

nit: leading space

@@ +513,3 @@
>      return;
>    }
> +  

nit: leading space

@@ +525,1 @@
>    

nit: leading space

@@ +832,5 @@
> +    ErrorResult ignored;
> +    ecb->Call(NS_LITERAL_STRING("Interrupted"), ignored);
> +    cancel = true;
> +  }
> +    

nit: leading space

::: dom/camera/test/test_camera.html
@@ +115,4 @@
>    },
>    shutter: function onShutter () {
>      Camera._shutter++;
>      

nit: leading space

::: dom/camera/test/test_camera_2.html
@@ +32,5 @@
> +  ok(false, "Error" + JSON.stringify(e));
> +}
> +
> +var capabilities = [ 'previewSizes', 'pictureSizes', 'fileFormats', 'maxFocusAreas', 'minExposureCompensation',
> +                     'maxExposureCompensation', 'stepExposureCompensation', 'maxMeteringAreas', 'videoSizes', 

nit: space in end of line.

@@ +94,5 @@
> +  takePictureSuccess: function taken_foto(blob) {
> +    var img = new Image();
> +    var test = this._currentTest;
> +    img.onload = function Imgsize() {
> +      ok(this.width == test.pictureSize.width, "The image taken has the width " + 

nit: space in end of line.

@@ +96,5 @@
> +    var test = this._currentTest;
> +    img.onload = function Imgsize() {
> +      ok(this.width == test.pictureSize.width, "The image taken has the width " + 
> +                                              this.width + " pictureSize width = " + test.pictureSize.width);
> +      ok(this.height == test.pictureSize.height, "The image taken has the height " + 

nit: space in end of line.

::: dom/webidl/CameraControl.webidl
@@ +258,5 @@
> +  /* the size of the thumbnail to be included in the picture returned
> +     by a call to takePicture(), assuming the chosen fileFormat supports
> +     one; an object with 'height' and 'width' properties that corresponds
> +     to one of the options returned by capabilities.pictureSizes.
> +     

nit: leading space.

::: widget/gonk/nativewindow/GonkNativeWindowICS.cpp
@@ +249,5 @@
> +                    case BufferSlot::DEQUEUED:
> +                        CNW_LOGD("dequeueBuffer: slot %d is DEQUEUED\n", i);
> +                        dequeuedCount++;
> +                        break;
> +                    

nit: leading space.

@@ +341,5 @@
>      // should touch it, except for freeAllBuffersLocked(); we handle that
>      // after trying to create the surface descriptor below.
>      //
>      // So we don't need mMutex locked, which would otherwise run the risk
>      // of a deadlock on calling AllocSurfaceDescriptorGralloc().    

nit: space in end of line.

Comment 90

[Ryan VanderMeulen [:RyanVM]]

(In reply to Phil Ringnalda (:philor) from comment #88)
> Backed out in
> https://hg.mozilla.org/integration/b2g-inbound/rev/f6db7c7f226f for
> https://tbpl.mozilla.org/php/getParsedLog.php?id=34455470&tree=B2g-Inbound -
> the good news is that you weren't actually getting the 34 assertions you
> expected in camera_2; the bad news is that you were shoving one assertion
> off into the next test, which is rarely a good sign.

I reverted bug 958200 from Gaia master as well.

Comment 91

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

Attachment: Clean-up of the CameraControl API, v28 [b2g-inbound] r=dhylands,jesup,jst,onecyrenus

(Hopefully) fix the DEBUG build mochitest assertion failures. This seems to address the problems locally.

try-server push: https://tbpl.mozilla.org/?tree=Try&rev=04894b78bac2

Comment 92

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

Attachment: Clean-up of the CameraControl API, v28.1 [b2g-inbound] r=dhylands,jesup,jst,onecyrenus

Rebased.

Comment 93

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

Take 2:
https://hg.mozilla.org/integration/b2g-inbound/rev/6e588c56764b

Comment 94

[Ryan VanderMeulen [:RyanVM]]

Backed out because the corresponding Gaia patches had to be reverted due to Gaia unit test failures on TBPL.
https://hg.mozilla.org/integration/b2g-inbound/rev/b47b10e405a5

gaia-unit-tests TEST-UNEXPECTED-FAIL | camera/test/unit/lib/sounds_test.js | Sounds Sounds#createAudio() Should set the mozAudioChannel type | expected false to be truthy
gaia-unit-tests TEST-UNEXPECTED-FAIL | camera/test/unit/controllers/hud_test.js | controllers/hud "before each" hook | The constructor should be a function.
TypeError: The constructor should be a function.

Comment 95

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

Take 3: https://hg.mozilla.org/integration/b2g-inbound/rev/e93e96ed8a5a

Comment 96

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/e93e96ed8a5a