1139027 - [B2G][Camera] Enable running of camera mochitests on B2G desktop

Status: RESOLVED FIXED

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

Description

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

With the advent of camera mochitests based on a JS camera implementation in bug 1062387, many of our tests are able to be run on B2G desktop in theory if the Gonk dependencies were isolated.

Comment 1

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

Attachment: mozconfig used for building B2G desktop with camera and code coverage

Comment 2

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

Attachment: WIP - Enable camera tests on B2G desktop v1

Currently suffers from a memory leak and breaks the normal build, but otherwise runs and passes the JS camera tests.

Comment 3

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

Attachment: WIP - Enable camera tests on B2G desktop v2

Fix build breaks for gonk builds.

Comment 4

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

Bug 1139721 fixes the memory leaks reported; as it turns out, not related to B2G desktop tests, just the first time it was noticed.

Comment 5

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

Attachment: Permit running of camera tests on B2G desktop v3

Comment 6

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

Comment on attachment 8574172 [details] [diff] [review]
Permit running of camera tests on B2G desktop v3

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

r+ with a bunch of nits fixed and an few issues addressed.

::: dom/camera/FallbackCameraPlatform.cpp
@@ +1,3 @@
> +/*
> + * Copyright (C) 2015 Mozilla Foundation
> + *

Is most of this copied from the AOSP file? If so, the license header should probably reflect that.

I'm not expert, but Gerv Markham can confirm whether or not anything else needs to be here.

::: dom/camera/FallbackCameraPlatform.h
@@ +73,5 @@
> +    sp()
> +      : mPtr(nullptr)
> +    { }
> +
> +    sp(T *aPtr)

nit: T*

@@ +77,5 @@
> +    sp(T *aPtr)
> +      : mPtr(aPtr)
> +    { }
> +
> +    virtual ~sp()

nit: consider |virtual ~sp() { }|

@@ +80,5 @@
> +
> +    virtual ~sp()
> +    { }
> +
> +    T* get() const {

nit: { on its own line, or make this a single-line function.

Ditto for all of the trivial functions below.

@@ +101,5 @@
> +
> +  enum error_t {
> +    OK = 0,
> +    UNKNOWN_ERROR,
> +    INVALID_OPERATION,

meh-nit: trailing comma on last item.

@@ +106,5 @@
> +  };
> +
> +  enum camera_msg_t {
> +    CAMERA_MSG_SHUTTER,
> +    CAMERA_MSG_COMPRESSED_IMAGE,

Ditto.

@@ +114,5 @@
> +  {
> +  public:
> +    String8() { }
> +
> +    String8(const char *aData)

nit: const char* aData

@@ +121,5 @@
> +    }
> +
> +    virtual ~String8() { }
> +
> +    const char* string() const {

nit: { on its own line.

@@ +138,5 @@
> +  struct CameraInfo {
> +    camera_facing_t facing;
> +  };
> +
> +  class Camera : public nsISupports

Should this get MOZ_FINAL?

@@ +155,5 @@
> +    int stopPreview() { return UNKNOWN_ERROR; }
> +    int startRecording() { return UNKNOWN_ERROR; }
> +    int stopRecording() { return UNKNOWN_ERROR; }
> +    int startFaceDetection() { return UNKNOWN_ERROR; }
> +    int stopFaceDetection() { return UNKNOWN_ERROR; }

nit: consider lining up all of the function bodies.

@@ +159,5 @@
> +    int stopFaceDetection() { return UNKNOWN_ERROR; }
> +
> +    static int32_t getNumberOfCameras() { return 2; }
> +
> +    static int getCameraInfo(int32_t aDevice, CameraInfo* aInfo) {

nit: { on its own line.

@@ +176,5 @@
> +
> +  protected:
> +    Camera() { }
> +    virtual ~Camera() { }
> +  };

Nuke the copy and assignment constructors too, please.

@@ +250,5 @@
> +    static const char KEY_VIDEO_STABILIZATION_SUPPORTED[];
> +    static const char KEY_LIGHTFX[];
> +  };
> +
> +  class MediaProfiles {

nit: { on its own line.

Also, if this class is not subclassed, make it MOZ_FINAL.

@@ +252,5 @@
> +  };
> +
> +  class MediaProfiles {
> +  public:
> +    static MediaProfiles* getInstance() {

Ditto, and for all the functions below.

::: dom/camera/GonkCameraHwMgr.h
@@ +50,3 @@
>  {
> +#ifndef MOZ_WIDGET_GONK
> +  NS_DECL_ISUPPORTS;

nit: no semi-colon at the end of this macro

::: dom/camera/GonkCameraParameters.cpp
@@ +77,5 @@
> +  nsCString* data = static_cast<nsCString*>(aUserArg);
> +  data->Append(aKey);
> +  data->Append('=');
> +  data->Append(*aValue);
> +  data->Append(';');

Is it okay if the set has a trailing semi-colon? (I assume it is, but... AOSP.)

@@ +106,5 @@
> +
> +    nsCString key(data, pos - data);
> +    data = pos + 1;
> +
> +    nsCString* value;

Use nsAutoCString for 'key' -- it includes a 64-byte stack-based buffer, which should be enough for all of the keys, and should be a bit faster.

::: dom/camera/GonkCameraParameters.h
@@ +21,3 @@
>  #include "nsTArray.h"
>  #include "nsString.h"
> +#include "mozilla/Mutex.h"

Switching to Mutexes will also address bug 1008483.

::: dom/camera/GonkRecorderProfiles.cpp
@@ +375,5 @@
>    }
>  
>    aProfiles.Clear();
>    profiles->EnumerateRead(Enumerate, static_cast<void*>(&aProfiles));
>    

nit: trailing whitespace.

::: dom/camera/TestGonkCameraHardware.cpp
@@ +30,5 @@
>  using namespace android;
>  using namespace mozilla;
>  using namespace mozilla::dom;
>  
> +#ifndef MOZ_WIDGET_GONK

ISUPPORTS seems orthogonal to the widget set. Is this intentional?

::: dom/camera/TestGonkCameraHardware.h
@@ +27,5 @@
>  class TestGonkCameraHardware : public android::GonkCameraHardware
>  {
> +#ifndef MOZ_WIDGET_GONK
> +  NS_DECL_ISUPPORTS_INHERITED;
> +#endif

nit: no semicolon at the end of this macro.

ISUPPORTS seems orthogonal to the widget set. Is this intentional?

::: dom/camera/test/test_bug1099390.html
@@ +25,5 @@
> +        ok(!gotCloseEvent, "gotCloseEvent was " + gotCloseEvent);
> +        ok(e.reason === "HardwareReleased", "'close' event reason is: " + e.reason);
> +        gotCloseEvent = true;
> +        if (gotReleasePromise) {
> +          resolve(); 

nit: trailing whitespace.

::: dom/camera/test/test_camera_bad_initial_config.html
@@ +24,2 @@
>  
> +    return navigator.mozCameras.getCamera(whichCamera, config);    

nit: trailing whitespace.

Comment 7

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

Attachment: Permit running of camera tests on B2G desktop v4 [carries r=mikeh]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8bc2bfe4c265

(In reply to Mike Habicher [:mikeh] from comment #6)
> Comment on attachment 8574172 [details] [diff] [review]
> Permit running of camera tests on B2G desktop v3
> 
> Review of attachment 8574172 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with a bunch of nits fixed and an few issues addressed.
> 
> ::: dom/camera/FallbackCameraPlatform.cpp
> @@ +1,3 @@
> > +/*
> > + * Copyright (C) 2015 Mozilla Foundation
> > + *
> 
> Is most of this copied from the AOSP file? If so, the license header should
> probably reflect that.
> 
> I'm not expert, but Gerv Markham can confirm whether or not anything else
> needs to be here.
> 
> ::: dom/camera/FallbackCameraPlatform.h
> @@ +73,5 @@
> > +    sp()
> > +      : mPtr(nullptr)
> > +    { }
> > +
> > +    sp(T *aPtr)
> 
> nit: T*
> 
> @@ +77,5 @@
> > +    sp(T *aPtr)
> > +      : mPtr(aPtr)
> > +    { }
> > +
> > +    virtual ~sp()
> 
> nit: consider |virtual ~sp() { }|
> 
> @@ +80,5 @@
> > +
> > +    virtual ~sp()
> > +    { }
> > +
> > +    T* get() const {
> 
> nit: { on its own line, or make this a single-line function.
> 
> Ditto for all of the trivial functions below.
> 
> @@ +101,5 @@
> > +
> > +  enum error_t {
> > +    OK = 0,
> > +    UNKNOWN_ERROR,
> > +    INVALID_OPERATION,
> 
> meh-nit: trailing comma on last item.
> 
> @@ +106,5 @@
> > +  };
> > +
> > +  enum camera_msg_t {
> > +    CAMERA_MSG_SHUTTER,
> > +    CAMERA_MSG_COMPRESSED_IMAGE,
> 
> Ditto.
> 
> @@ +114,5 @@
> > +  {
> > +  public:
> > +    String8() { }
> > +
> > +    String8(const char *aData)
> 
> nit: const char* aData
> 
> @@ +121,5 @@
> > +    }
> > +
> > +    virtual ~String8() { }
> > +
> > +    const char* string() const {
> 
> nit: { on its own line.
> 
> @@ +138,5 @@
> > +  struct CameraInfo {
> > +    camera_facing_t facing;
> > +  };
> > +
> > +  class Camera : public nsISupports
> 
> Should this get MOZ_FINAL?
> 
> @@ +155,5 @@
> > +    int stopPreview() { return UNKNOWN_ERROR; }
> > +    int startRecording() { return UNKNOWN_ERROR; }
> > +    int stopRecording() { return UNKNOWN_ERROR; }
> > +    int startFaceDetection() { return UNKNOWN_ERROR; }
> > +    int stopFaceDetection() { return UNKNOWN_ERROR; }
> 
> nit: consider lining up all of the function bodies.
> 
> @@ +159,5 @@
> > +    int stopFaceDetection() { return UNKNOWN_ERROR; }
> > +
> > +    static int32_t getNumberOfCameras() { return 2; }
> > +
> > +    static int getCameraInfo(int32_t aDevice, CameraInfo* aInfo) {
> 
> nit: { on its own line.
> 
> @@ +176,5 @@
> > +
> > +  protected:
> > +    Camera() { }
> > +    virtual ~Camera() { }
> > +  };
> 
> Nuke the copy and assignment constructors too, please.
> 
> @@ +250,5 @@
> > +    static const char KEY_VIDEO_STABILIZATION_SUPPORTED[];
> > +    static const char KEY_LIGHTFX[];
> > +  };
> > +
> > +  class MediaProfiles {
> 
> nit: { on its own line.
> 
> Also, if this class is not subclassed, make it MOZ_FINAL.
> 
> @@ +252,5 @@
> > +  };
> > +
> > +  class MediaProfiles {
> > +  public:
> > +    static MediaProfiles* getInstance() {
> 
> Ditto, and for all the functions below.
> 
> ::: dom/camera/GonkCameraHwMgr.h
> @@ +50,3 @@
> >  {
> > +#ifndef MOZ_WIDGET_GONK
> > +  NS_DECL_ISUPPORTS;
> 
> nit: no semi-colon at the end of this macro
> 
> @@ +106,5 @@
> > +
> > +    nsCString key(data, pos - data);
> > +    data = pos + 1;
> > +
> > +    nsCString* value;
> 
> Use nsAutoCString for 'key' -- it includes a 64-byte stack-based buffer,
> which should be enough for all of the keys, and should be a bit faster.
> 
> ::: dom/camera/GonkRecorderProfiles.cpp
> @@ +375,5 @@
> >    }
> >  
> >    aProfiles.Clear();
> >    profiles->EnumerateRead(Enumerate, static_cast<void*>(&aProfiles));
> >    
> 
> nit: trailing whitespace.
> 
> ::: dom/camera/test/test_bug1099390.html
> @@ +25,5 @@
> > +        ok(!gotCloseEvent, "gotCloseEvent was " + gotCloseEvent);
> > +        ok(e.reason === "HardwareReleased", "'close' event reason is: " + e.reason);
> > +        gotCloseEvent = true;
> > +        if (gotReleasePromise) {
> > +          resolve(); 
> 
> nit: trailing whitespace.
> 
> ::: dom/camera/test/test_camera_bad_initial_config.html
> @@ +24,2 @@
> >  
> > +    return navigator.mozCameras.getCamera(whichCamera, config);    
> 
> nit: trailing whitespace.

Done.

> ::: dom/camera/GonkCameraParameters.cpp
> @@ +77,5 @@
> > +  nsCString* data = static_cast<nsCString*>(aUserArg);
> > +  data->Append(aKey);
> > +  data->Append('=');
> > +  data->Append(*aValue);
> > +  data->Append(';');
> 
> Is it okay if the set has a trailing semi-colon? (I assume it is, but...
> AOSP.)
> 

Yes, the parser handles this just fine. But I fixed it anyways.

> ::: dom/camera/GonkCameraParameters.h
> @@ +21,3 @@
> >  #include "nsTArray.h"
> >  #include "nsString.h"
> > +#include "mozilla/Mutex.h"
> 
> Switching to Mutexes will also address bug 1008483.
> 

Only partially. There remains another RW lock in CameraControlImpl.

> ::: dom/camera/TestGonkCameraHardware.cpp
> @@ +30,5 @@
> >  using namespace android;
> >  using namespace mozilla;
> >  using namespace mozilla::dom;
> >  
> > +#ifndef MOZ_WIDGET_GONK
> 
> ISUPPORTS seems orthogonal to the widget set. Is this intentional?
> 
> ::: dom/camera/TestGonkCameraHardware.h
> @@ +27,5 @@
> >  class TestGonkCameraHardware : public android::GonkCameraHardware
> >  {
> > +#ifndef MOZ_WIDGET_GONK
> > +  NS_DECL_ISUPPORTS_INHERITED;
> > +#endif
> 
> nit: no semicolon at the end of this macro.
> 
> ISUPPORTS seems orthogonal to the widget set. Is this intentional?
> 

Yes. On Gonk platforms, we put GonkCameraHardware and TestGonkCameraHardware in AOSP smart pointers, and on non-Gonk platforms (i.e. B2G desktop), we put them in Mozilla smart pointers. It is better to compile out support for the latter on the former platform to avoid any temptation.

Comment 8

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

Removing bug 1139721 as a dependency as the memory leak is already present in other builds and unrelated to B2G desktop testing changes.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

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

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1a02c68523da