940424 - [Camera][Test] Additional tests for CameraControl API

Status: RESOLVED FIXED

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

Description

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

Thinking: use SpecialPowers to switch the bottom end of the CameraControl stack to a fake camera into which we can inject various failure conditions.

Comment 1

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

Attachment: WIP - testable hardware API, v1 [b2g-inbound]

First cut of a failable GonkCameraHardware layer than can be used to test the error-handling paths in the upper layers. This patch includes a number of fixes to the upper layers found just by failing to properly initialize the camera hardware. :)

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

Comment 2

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

Attachment: WIP - testable hardware API, v1.1 [b2g-inbound]

Fix issues caught by the try-server.
New push: https://tbpl.mozilla.org/?tree=Try&rev=6ad79ed30fc7

Comment 3

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

latest try-server results: https://tbpl.mozilla.org/?tree=Try&rev=26abbe97e4aa
(Orange is due to another, unrelated test failing.)

Comment 4

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

Attachment: WIP - testable hardware API, v2 [b2g-inbound]

Comment 5

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

Attachment: WIP - testable hardware API, v3 [b2g-inbound]

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

Comment 6

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

Attachment: WIP

Comment 7

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

Attachment: Testable hardware API, v4 [b2g-inbound]

Try not to hit [Enter] too quickly this time.

Comment 8

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

Attachment: Part 1 - Camera, v1

Comment 9

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

Attachment: Part 2 - DOM, v1

These are changes to the DOM interface layer, but they only involve the bottom end.

Comment 10

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

Attachment: Part 3 - Test, v1

Comment 11

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

Attachment: Part 4 - WebRTC, v1

In short, separated camera creation from starting/stopping for testability, which affected how WebRTC talks to the camera.

Comment 12

[Dave Hylands [:dhylands]]

Comment on attachment 8367630 [details] [diff] [review]
Part 1 - Camera, v1

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

::: 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 */

debug?

::: dom/camera/CameraControlImpl.cpp
@@ +234,5 @@
>    // This callback can run on threads other than the Main Thread and
>    //  the Camera Thread.
>    RwLockAutoEnterRead lock(mListenerLock);
>  
> +// #ifdef PR_LOGGING

rather than commenting these out, introduce another LOGGING macro and use that instead (or use #if defined(PR_LOGGING) || defined(STDERR_LOGGING)

@@ +259,5 @@
> +  };
> +  if (static_cast<unsigned int>(aError) < sizeof(error) / sizeof(error[0]) &&
> +    static_cast<unsigned int>(aContext) < sizeof(context) / sizeof(context[0])) {
> +    DOM_CAMERA_LOGW("CameraControlImpl::OnError : aContext='%s', aError='%s'\n",
> +      context[aContext], error[aError]);

I usually like to log the integer values as well, just in case there is ever a disagreement between the integer value and the associated string.

::: dom/camera/CameraControlListener.h
@@ +20,5 @@
> +  CameraControlListener()
> +  {
> +    MOZ_COUNT_CTOR(CameraControlListener);
> +  }
> +    

nit: trailing space

::: dom/camera/GonkCameraControl.cpp
@@ +101,5 @@
> +  MOZ_ASSERT(NS_GetCurrentThread() == mCameraThread);
> +
> +  nsresult rv = Initialize();
> +  if (NS_FAILED(rv)) {
> +    return rv;

Log failure? How likely is this to happen?

@@ +107,5 @@
> +
> +  if (aInitialConfig) {
> +    rv = SetConfigurationInternal(*aInitialConfig);
> +    if (NS_FAILED(rv)) {
> +      // The initial configuration failed, close up the hardware

Log failure?

::: dom/camera/ICameraControl.h
@@ +117,5 @@
>      Size      mPreviewSize;
>      nsString  mRecorderProfile;
>    };
> +  static already_AddRefed<ICameraControl> Create(uint32_t aCameraId);
> +  

nit: trailing space

::: dom/camera/TestGonkCameraHardware.cpp
@@ +52,5 @@
> +const nsAdoptingCString&
> +TestGonkCameraHardware::TestCase()
> +{
> +  const nsAdoptingCString& test = Preferences::GetCString("camera.control.test.hardware");
> +  return test;

This looks bad to me.

The lifetime of the temporary returned by Preferences::GetCString will extend to the lifetime of test.
test goes out of scope when this function returns, so the returned reference is invalid.

Comment 13

[Dave Hylands [:dhylands]]

Comment on attachment 8367631 [details] [diff] [review]
Part 2 - DOM, v1

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

::: dom/camera/DOMCameraControlListener.cpp
@@ +17,5 @@
> +                                                   CameraPreviewMediaStream* aStream)
> +  : mDOMCameraControl(new nsMainThreadPtrHolder<nsDOMCameraControl>(aDOMCameraControl))
> +  , mStream(aStream)
> +{
> +  DOM_CAMERA_LOGT("%s:%d : this=%p, camera=%p, stream=%p\n",

You could hide the __func__ and __LINE__ stuff in the DOM_CAMERA_LOGT macro so you don't need to add them everytime you call them.

Comment 14

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

Attachment: Testable hardware API, v5 [b2g-inbound]

Comment 15

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

Attachment: Part 1 - Camera, v2

Incorporate review feedback.

Comment 16

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

Attachment: Testable hardware API, v6 [b2g-inbound]

Attach the right patch this time.

Comment 17

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

Attachment: Part 1 - Camera, v3

Attach the right patch this time.

Comment 18

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

Attachment: Testable hardware API, v6.1 [b2g-inbound]

try-server results:
- emulator+tests: https://tbpl.mozilla.org/?tree=Try&rev=19e7a663f8a0
- win32+macosx: https://tbpl.mozilla.org/?tree=Try&rev=0eb436123ebc

Comment 19

[[e-mail] [:onecyrenus]]

Comment on attachment 8367632 [details] [diff] [review]
Part 3 - Test, v1

One nit would be to document the usage of camera.control.test.enabled & camera.control.test.hardware. 

I see where it is happening in the source, but there seems to be a mix of previous functionality.

Comment 20

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

(In reply to [e-mail]  [:onecyrenus] from comment #19)
> 
> One nit would be to document the usage of camera.control.test.enabled &
> camera.control.test.hardware. 
> 
> I see where it is happening in the source, but there seems to be a mix of
> previous functionality.

Thanks for the feedback. Is there a good out-of-band place to document the test prefs, or do you just mean somewhere in the test and/or code?

Comment 21

[[e-mail] [:onecyrenus]]

It would be great if there was an out of band place for test prefs, but i  just meant in the test to explain the purpose of the tests.

Comment 22

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

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

Comment 23

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

Attachment: Testable hardware API, v6.2 [b2g-inbound] r=dclarke,dhylands,jesup

Carrying r+s forward; added notes on camera test prefs to camera_common.js, as requested.

Comment 24

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

Attachment: Testable hardware API, v6.3 [b2g-inbound] r=dclarke,dhylands,jesup

Correcting hiccups found in previous try run.

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

Comment 25

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

Attachment: Testable hardware API, v6.4 [b2g-inbound] r=dclarke,dhylands,jesup

Applies on top of attachment 8378529 [details] [diff] [review]. Carrying r+ forward.

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

Comment 26

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

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

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4ab6a5b763d8