[Camera][Test] Additional tests for CameraControl API

RESOLVED FIXED

Status

Firefox OS
Gaia::Camera
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: mikeh, Assigned: mikeh)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 17 obsolete attachments)

72.36 KB, patch
mikeh
: review+
Details | Diff | Splinter Review
Thinking: use SpecialPowers to switch the bottom end of the CameraControl stack to a fake camera into which we can inject various failure conditions.
Summary: Additional tests for CameraControl API → [Camera][Test] Additional tests for CameraControl API
Created attachment 8361230 [details] [diff] [review]
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
Assignee: nobody → mhabicher
Status: NEW → ASSIGNED
Created attachment 8361330 [details] [diff] [review]
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
Attachment #8361230 - Attachment is obsolete: true
latest try-server results: https://tbpl.mozilla.org/?tree=Try&rev=26abbe97e4aa
(Orange is due to another, unrelated test failing.)
Created attachment 8363859 [details] [diff] [review]
WIP - testable hardware API, v2 [b2g-inbound]
Attachment #8361330 - Attachment is obsolete: true
Created attachment 8365166 [details] [diff] [review]
WIP - testable hardware API, v3 [b2g-inbound]

try-server push: https://tbpl.mozilla.org/?tree=Try&rev=21e8733fc216
Attachment #8363859 - Attachment is obsolete: true
Created attachment 8367626 [details] [diff] [review]
Testable hardware API, v4 [b2g-inbound]

Try not to hit [Enter] too quickly this time.
Attachment #8365166 - Attachment is obsolete: true
Attachment #8367624 - Attachment is obsolete: true
Created attachment 8367630 [details] [diff] [review]
Part 1 - Camera, v1
Attachment #8367630 - Flags: review?(dhylands)
Created attachment 8367631 [details] [diff] [review]
Part 2 - DOM, v1

These are changes to the DOM interface layer, but they only involve the bottom end.
Attachment #8367631 - Flags: review?(dhylands)
Created attachment 8367632 [details] [diff] [review]
Part 3 - Test, v1
Attachment #8367632 - Flags: review?(dclarke)
Created attachment 8367633 [details] [diff] [review]
Part 4 - WebRTC, v1

In short, separated camera creation from starting/stopping for testability, which affected how WebRTC talks to the camera.
Attachment #8367633 - Flags: review?(rjesup)

Updated

5 years ago
Attachment #8367633 - Flags: review?(rjesup) → review+
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.
Attachment #8367630 - Flags: review?(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.
Attachment #8367631 - Flags: review?(dhylands) → review+
Created attachment 8368725 [details] [diff] [review]
Testable hardware API, v5 [b2g-inbound]
Attachment #8367626 - Attachment is obsolete: true
Created attachment 8368726 [details] [diff] [review]
Part 1 - Camera, v2

Incorporate review feedback.
Attachment #8367630 - Attachment is obsolete: true
Attachment #8368726 - Flags: review?(dhylands)
Created attachment 8368728 [details] [diff] [review]
Testable hardware API, v6 [b2g-inbound]

Attach the right patch this time.
Attachment #8368725 - Attachment is obsolete: true
Created attachment 8368731 [details] [diff] [review]
Part 1 - Camera, v3

Attach the right patch this time.
Attachment #8368726 - Attachment is obsolete: true
Attachment #8368726 - Flags: review?(dhylands)
Attachment #8368731 - Flags: review?(dhylands)
Attachment #8368731 - Flags: review?(dhylands) → review+
Created attachment 8368910 [details] [diff] [review]
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
Attachment #8368728 - Attachment is obsolete: true
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.
Attachment #8367632 - Flags: review?(dclarke) → review+
(In reply to dclarke@mozilla.com  [: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?
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.
Target Milestone: --- → 1.4 S2 (28feb)
Blocks: 972120
try-server push: https://tbpl.mozilla.org/?tree=Try&rev=31f14dfdf859
Target Milestone: 1.4 S2 (28feb) → ---
Created attachment 8377845 [details] [diff] [review]
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.
Attachment #8367631 - Attachment is obsolete: true
Attachment #8367632 - Attachment is obsolete: true
Attachment #8367633 - Attachment is obsolete: true
Attachment #8368731 - Attachment is obsolete: true
Attachment #8368910 - Attachment is obsolete: true
Attachment #8377845 - Flags: review+
Created attachment 8378292 [details] [diff] [review]
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
Attachment #8377845 - Attachment is obsolete: true
Attachment #8378292 - Flags: review+
Created attachment 8378705 [details] [diff] [review]
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
Attachment #8378292 - Attachment is obsolete: true
Attachment #8378705 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/4ab6a5b763d8
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.