Closed Bug 740997 Opened 8 years ago Closed 7 years ago

ICS camera support

Categories

(Firefox OS Graveyard :: General, defect)

All
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-kilimanjaro:+, blocking-basecamp:+)

RESOLVED FIXED
blocking-kilimanjaro +
blocking-basecamp +

People

(Reporter: fabrice, Assigned: mikeh)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-needed)

Attachments

(1 file, 76 obsolete files)

156.34 KB, patch
Details | Diff | Splinter Review
Attached patch wip (obsolete) — Splinter Review
With this patch, I can start the preview, but closing it leads to a hang:

Program received signal SIGINT, Interrupt.
__futex_syscall3 () at bionic/libc/arch-arm/bionic/atomics_arm.S:182
182	    swi     #0
(gdb) bt
#0  __futex_syscall3 () at bionic/libc/arch-arm/bionic/atomics_arm.S:182
#1  0x400cb6a4 in __pthread_cond_timedwait_relative (cond=0x1b40d70, mutex=0x1b40d68, reltime=0x0) at bionic/libc/bionic/pthread.c:1482
#2  0x400cb758 in __pthread_cond_timedwait (cond=0x1b40d70, mutex=0x1b40d68, abstime=0x0, clock=0) at bionic/libc/bionic/pthread.c:1505
#3  0x42f6adba in wait (mutex=..., this=<optimized out>) at frameworks/base/include/utils/threads.h:455
#4  android::CameraHardwareSec::stopPreviewInternal (this=0x1b40d48) at device/samsung/crespo/libcamera/SecCameraHWInterface.cpp:744
#5  0x42f6adf0 in android::CameraHardwareSec::stopPreview (this=0x1b40d48) at device/samsung/crespo/libcamera/SecCameraHWInterface.cpp:758
#6  0x42f685f2 in android::HAL_camera_device_stop_preview (dev=<optimized out>) at device/samsung/crespo/libcamera/SecCameraHWInterface.cpp:2412
#7  0x405c341c in stopPreview (this=<optimized out>) at /home/fabrice/dev/b2g/central/netwerk/protocol/device/gonk/CameraHardwareInterfaceICS.h:218
#8  CameraICS::stopPreview (this=0x4b9e3640) at /home/fabrice/dev/b2g/central/netwerk/protocol/device/GonkCaptureProvider.cpp:227
#9  0x405c318e in GonkCameraInputStream::doClose (this=0x4bdd5f00)
    at /home/fabrice/dev/b2g/central/netwerk/protocol/device/GonkCaptureProvider.cpp:631
#10 0x405c31b8 in GonkCameraInputStream::CloseWithStatus (this=0x1b40d70, status=128)
    at /home/fabrice/dev/b2g/central/netwerk/protocol/device/GonkCaptureProvider.cpp:678

Looking at SecCameraHWInterface.cpp I don't see why this should deadlock, and CameraService.cpp shutdown code code is similar to ours (see CameraService::Client::disconnect).
I can take a look at this.  What hardware is this happening on?
(In reply to Mike Habicher [:mikeh] from comment #1)
> I can take a look at this.  What hardware is this happening on?

I only tested on a nexus s.
(In reply to Fabrice Desré [:fabrice] from comment #2)
> (In reply to Mike Habicher [:mikeh] from comment #1)
> > I can take a look at this.  What hardware is this happening on?
> 
> I only tested on a nexus s.

Okay.  I'm still waiting on one up here in Toronto.
Blocks: gonk-ics
Found the (a?) mutex where the camera hangs (prints are in bionic/pthread.c):

I/Gecko   (   87): XxXxX HW before ops->stop_preview
F/mikeh   (   87): <<< [1483] 0x12e1200/0x12e1208
F/mikeh   (   87):     [1485] 0x12e1200/0x12e1208

pthread.c:
int __pthread_cond_timedwait_relative(pthread_cond_t *cond,
                                      pthread_mutex_t * mutex,
                                      const struct timespec *reltime)
{
    int  status;
    int  oldvalue = cond->value;

    __libc_android_log_print( ANDROID_LOG_FATAL, "mikeh", "<<< [%4d] %p/%p", __LINE__, mutex, cond );
    pthread_mutex_unlock(mutex);
    __libc_android_log_print( ANDROID_LOG_FATAL, "mikeh", "    [%4d] %p/%p", __LINE__, mutex, cond );
    status = __futex_wait_ex(&cond->value, COND_IS_SHARED(cond), oldvalue, reltime);
    __libc_android_log_print( ANDROID_LOG_FATAL, "mikeh", "    [%4d] %p/%p", __LINE__, mutex, cond );
    pthread_mutex_lock(mutex);
    __libc_android_log_print( ANDROID_LOG_FATAL, "mikeh", ">>> [%4d] %p/%p", __LINE__, mutex, cond );

    if (status == (-ETIMEDOUT)) return ETIMEDOUT;
    return 0;
}
Preview frame callback blocks on monitor in GonkCameraInputStream::ReceiveFrame():

  printf_stderr("mikeh-trace: %s:%d\n", __func__, __LINE__);

  {
    ReentrantMonitorAutoEnter enter(mMonitor);
    mAvailable += mFrameSize;
    mFrameQueue.Push((void*)fullFrame);
  }

  printf_stderr("mikeh-trace: %s:%d\n", __func__, __LINE__);

Once the Back button is tapped, the callback hits the first printf(), but never the second one.
Confirmed that the conflicting mMonitor entry is in GonkCameraInputStream::doClose().  With a guard against this condition, now seeing an abort in dlfree():

(gdb) bt
#0  __libc_android_abort () at bionic/libc/unistd/abort.c:82
#1  0x400d48c2 in __bionic_heap_error (msg=0x400fb188 "INVALID HEAP ADDRESS", function=0x400fe6c4 "dlfree") at bionic/libc/bionic/dlmalloc.c:2291
#2  0x400d6c00 in dlfree (mem=0x4c9a50d0) at bionic/libc/bionic/dlmalloc.c:4459
#3  0x400d7276 in free (mem=0xdeadbaad) at bionic/libc/bionic/malloc_debug_common.c:223
#4  0x401119ae in operator delete (ptr=0xdeadbaad) at bionic/libstdc++/src/new.cpp:51
#5  0x4171feea in android::MemoryBase::~MemoryBase (this=0x4c9a50d0, __in_chrg=<optimized out>, __vtt_parm=<optimized out>) at frameworks/base/libs/binder/MemoryBase.cpp:43
#6  0x4171ffe6 in virtual thunk to android::MemoryBase::~MemoryBase() () at frameworks/base/libs/binder/MemoryBase.cpp:46
#7  0x414f6076 in android::RefBase::decStrong (this=0x4c9a50ec, id=0x4ccaf338) at frameworks/base/libs/utils/RefBase.cpp:359
#8  0x40598f3c in ~sp (this=<optimized out>, __in_chrg=<optimized out>) at /home/mikeh/devel/mozilla/b2g4/glue/gonk-ics/frameworks/base/include/utils/StrongPointer.h:149
#9  android::CameraHardwareInterface_ICS::CameraHeapMemory::~CameraHeapMemory (this=0x4c7c1dc0, __in_chrg=<optimized out>)
    at /home/mikeh/devel/mozilla/b2g4/gecko/netwerk/protocol/device/gonk/CameraHardwareInterfaceICS.h:535
#10 0x40598f76 in android::CameraHardwareInterface_ICS::CameraHeapMemory::~CameraHeapMemory (this=0xdeadbaad, __in_chrg=<optimized out>)
    at /home/mikeh/devel/mozilla/b2g4/gecko/netwerk/protocol/device/gonk/CameraHardwareInterfaceICS.h:536
#11 0x414f6076 in android::RefBase::decStrong (this=0x4c7c1dc0, id=0x4c7c1dc0) at frameworks/base/libs/utils/RefBase.cpp:359
#12 0x40598e2a in android::CameraHardwareInterface_ICS::__put_memory (data=<optimized out>)
    at /home/mikeh/devel/mozilla/b2g4/gecko/netwerk/protocol/device/gonk/CameraHardwareInterfaceICS.h:564
#13 0x43c900a8 in android::CameraHardwareSec::release (this=0xd632e0) at device/samsung/crespo/libcamera/SecCameraHWInterface.cpp:2288
#14 0x43c8f62e in android::HAL_camera_device_release (dev=0xd580a0) at device/samsung/crespo/libcamera/SecCameraHWInterface.cpp:2610
#15 0x405983b0 in release (this=<optimized out>) at /home/mikeh/devel/mozilla/b2g4/gecko/netwerk/protocol/device/gonk/CameraHardwareInterfaceICS.h:431
#16 CameraICS::release (this=<optimized out>) at /home/mikeh/devel/mozilla/b2g4/gecko/netwerk/protocol/device/GonkCaptureProvider.cpp:232
#17 0x405987f4 in GonkCameraInputStream::doClose (this=0x4ccdce00) at /home/mikeh/devel/mozilla/b2g4/gecko/netwerk/protocol/device/GonkCaptureProvider.cpp:644
#18 0x4059881c in GonkCameraInputStream::CloseWithStatus (this=0xdeadbaad, status=1) at /home/mikeh/devel/mozilla/b2g4/gecko/netwerk/protocol/device/GonkCaptureProvider.cpp:690
With a guard around the deadlock, am now looking into what appears to be an invalid pointer/double-free problem:

(gdb) bt
#0  __libc_android_abort () at bionic/libc/unistd/abort.c:82
#1  0x400d48c2 in __bionic_heap_error (msg=0x400fb188 "INVALID HEAP ADDRESS", function=0x400fe6c4 "dlfree") at bionic/libc/bionic/dlmalloc.c:2291
#2  0x400d6c00 in dlfree (mem=0x4c9a50d0) at bionic/libc/bionic/dlmalloc.c:4459
#3  0x400d7276 in free (mem=0xdeadbaad) at bionic/libc/bionic/malloc_debug_common.c:223
#4  0x401119ae in operator delete (ptr=0xdeadbaad) at bionic/libstdc++/src/new.cpp:51
#5  0x4171feea in android::MemoryBase::~MemoryBase (this=0x4c9a50d0, __in_chrg=<optimized out>, __vtt_parm=<optimized out>) at frameworks/base/libs/binder/MemoryBase.cpp:43
#6  0x4171ffe6 in virtual thunk to android::MemoryBase::~MemoryBase() () at frameworks/base/libs/binder/MemoryBase.cpp:46
#7  0x414f6076 in android::RefBase::decStrong (this=0x4c9a50ec, id=0x4ccaf338) at frameworks/base/libs/utils/RefBase.cpp:359
#8  0x40598f3c in ~sp (this=<optimized out>, __in_chrg=<optimized out>) at /home/mikeh/devel/mozilla/b2g4/glue/gonk-ics/frameworks/base/include/utils/StrongPointer.h:149
#9  android::CameraHardwareInterface_ICS::CameraHeapMemory::~CameraHeapMemory (this=0x4c7c1dc0, __in_chrg=<optimized out>)
    at /home/mikeh/devel/mozilla/b2g4/gecko/netwerk/protocol/device/gonk/CameraHardwareInterfaceICS.h:535
#10 0x40598f76 in android::CameraHardwareInterface_ICS::CameraHeapMemory::~CameraHeapMemory (this=0xdeadbaad, __in_chrg=<optimized out>)
    at /home/mikeh/devel/mozilla/b2g4/gecko/netwerk/protocol/device/gonk/CameraHardwareInterfaceICS.h:536
#11 0x414f6076 in android::RefBase::decStrong (this=0x4c7c1dc0, id=0x4c7c1dc0) at frameworks/base/libs/utils/RefBase.cpp:359
#12 0x40598e2a in android::CameraHardwareInterface_ICS::__put_memory (data=<optimized out>)
    at /home/mikeh/devel/mozilla/b2g4/gecko/netwerk/protocol/device/gonk/CameraHardwareInterfaceICS.h:564
#13 0x43c900a8 in android::CameraHardwareSec::release (this=0xd632e0) at device/samsung/crespo/libcamera/SecCameraHWInterface.cpp:2288
#14 0x43c8f62e in android::HAL_camera_device_release (dev=0xd580a0) at device/samsung/crespo/libcamera/SecCameraHWInterface.cpp:2610
#15 0x405983b0 in release (this=<optimized out>) at /home/mikeh/devel/mozilla/b2g4/gecko/netwerk/protocol/device/gonk/CameraHardwareInterfaceICS.h:431
#16 CameraICS::release (this=<optimized out>) at /home/mikeh/devel/mozilla/b2g4/gecko/netwerk/protocol/device/GonkCaptureProvider.cpp:232
#17 0x405987f4 in GonkCameraInputStream::doClose (this=0x4ccdce00) at /home/mikeh/devel/mozilla/b2g4/gecko/netwerk/protocol/device/GonkCaptureProvider.cpp:644
#18 0x4059881c in GonkCameraInputStream::CloseWithStatus (this=0xdeadbaad, status=1) at /home/mikeh/devel/mozilla/b2g4/gecko/netwerk/protocol/device/GonkCaptureProvider.cpp:690
Applied this patch for maguro device. And am getting a segfault:

Program received signal SIGSEGV, Segmentation fault.
android::CameraHardwareInterface_ICS::__dequeue_buffer (w=<value optimized out>, buffer=0xbe9bd2fc, stride=0xbe9bd304)
    at /local/mnt/workspace/ikumar/ics.b2g/b2g/gecko/netwerk/protocol/device/gonk/CameraHardwareInterfaceICS.h:596
596                  *stride = anb->stride;
(gdb) bt
#0  android::CameraHardwareInterface_ICS::__dequeue_buffer (w=<value optimized out>, buffer=0xbe9bd2fc, 
    stride=0xbe9bd304)
    at /local/mnt/workspace/ikumar/ics.b2g/b2g/gecko/netwerk/protocol/device/gonk/CameraHardwareInterfaceICS.h:596

Seems like it's because a dummy ANativeWindow implementation is used in CameraICS constructor which is passed on by mCamera->setPreviewWindow(mWindow) in startPreview() call. __dequeue_buffer() in CameraHardwareInterfaceICS.h is using this dummy implementation of ANativeWindow  (a->dequeueBuffer) which returns 0 for the ANativeWindowBuffer resulting in the seg fault.

Does this dummy implementation gets overwritten before the use in nexus s?
(In reply to Inder from comment #8)

> 
> Does this dummy implementation gets overwritten before the use in nexus s?

No it's not. But we don't do anything at all in WindowQueueBuffer and WindowDequeueBuffer, so that may cause your issues.

Also, modifying CameraHardwareInterfaceICS.h looks perfectly ok to me.
Comment on attachment 614391 [details] [diff] [review]
Fix for the camera-closing deadlock, and temporary (MEMORY-LEAKING!) fix for the SIGABRT

Note that this includes the changes from Fabrice's 'wip' patch.
Assignee: nobody → mhabicher
Added the implementation of ANativeWindow instead of the stub implementation to make Qualcomm camera implementation happy but the preview is not working. I can see the UI come up but no preview. And, when I close the preview I see data abort.
Below is the logcat output when I start the preview. Any clues will be much appreciated.
NOTE: This patch includes previous changes by Fabrice and Mike.
---------------------------
Logcat:
E/mm-camera(   86): Waiting for frame thread to start ! 
V/QualcommCameraHardware(   86): runFrameThread E
E/mm-camera(   86): Wait over, frame thread ready !!!! 
V/QualcommCameraHardware(   86): initPreview X: 1
I/QualcommCameraHardware(   86): preview_thread E
V/QualcommCameraHardware(   86): startPreviewInternal X
I/QualcommCameraHardware(   86):  getBuffersAndStartPreview : X 
V/QualcommCameraHardware(   86): startPreview X
E/QualcommCamera(   86): Qint android::start_preview(camera_device*): X
I/QualcommCameraHardware(   86): receivePreviewFrame E
I/QualcommCameraHardware(   86): receivePreviewFrame X
I/QualcommCameraHardware(   86): receivePreviewFrame E
I/QualcommCameraHardware(   86): receivePreviewFrame X
I/Gecko   (   86): virtual android::CameraHardwareInterface_ICS::CameraHeapMemory::~CameraHeapMemory(): WARNING -- memory leak, see bug 740997
I/QualcommCameraHardware(   86): receivePreviewFrame E
I/QualcommCameraHardware(   86): receivePreviewFrame X
I/QualcommCameraHardware(   86): receivePreviewFrame E
I/QualcommCameraHardware(   86): receivePreviewFrame X
I/Gecko   (   86): virtual android::CameraHardwareInterface_ICS::CameraHeapMemory::~CameraHeapMemory(): WARNING -- memory leak, see bug 740997
I/QualcommCameraHardware(   86): receivePreviewFrame E
I/QualcommCameraHardware(   86): receivePreviewFrame X
I/Gecko   (   86): Observed notification ril-radiostate-changed
I/Gecko   (   86): Radio state (signalStrength) changed.
I/Gecko   (   86): Radio state (bars) changed.
I/Gecko   (   86): virtual android::CameraHardwareInterface_ICS::CameraHeapMemory::~CameraHeapMemory(): WARNING -- memory leak, see bug 740997
I/Gecko   (   86): *** AUS:SVC Checker:getUpdateURL - update URL: http://update.boot2gecko.org/m2.5/updates.xml
E/GeckoConsole(   86): AUS:SVC Checker:getUpdateURL - update URL: http://update.boot2gecko.org/m2.5/updates.xml
I/Gecko   (   86): *** AUS:SVC gCanCheckForUpdates - able to check for updates
E/GeckoConsole(   86): AUS:SVC gCanCheckForUpdates - able to check for updates
I/Gecko   (   86): *** AUS:SVC Checker:checkForUpdates - sending request to: http://update.boot2gecko.org/m2.5/updates.xml
E/GeckoConsole(   86): AUS:SVC Checker:checkForUpdates - sending request to: http://update.boot2gecko.org/m2.5/updates.xml
I/Gecko   (   86): *** UTM:SVC TimerManager:notify - notified @mozilla.org/updates/update-service;1
E/GeckoConsole(   86): UTM:SVC TimerManager:notify - notified @mozilla.org/updates/update-service;1
I/Gecko   (   86): *** AUS:SVC Checker:onError - request.status: 2152398878
E/GeckoConsole(   86): AUS:SVC Checker:onError - request.status: 2152398878
I/Gecko   (   86): *** AUS:SVC getStatusTextFromCode - transfer error: Update server not found (check your internet connection), code: 2152398878
E/GeckoConsole(   86): AUS:SVC getStatusTextFromCode - transfer error: Update server not found (check your internet connection), code: 2152398878
I/Gecko   (   86): *** AUS:SVC UpdateService:notify:listener - error during background update: Update server not found (check your internet connection)
E/GeckoConsole(   86): AUS:SVC UpdateService:notify:listener - error during background update: Update server not found (check your internet connection)
I/ONCRPC  (   91): rpc_handle_rpc_call: for Xid: 69, Prog: 31000000, Vers: fc37ad5c, Proc: 00000012
I/ONCRPC  (   91): rpc_handle_rpc_call: Find Status: 0 Xid: 69
I/ONCRPC  (   91): oncrpc_proxy_handle_cmd_rpc_call: Dispatching xid: 69
Mike, does your patch here get preview working on the Nexus S?

Inder, is the camera HAL itself actually writing data into our preview surface?  Also, can you attach a backtrace from the crash?
(In reply to Chris Jones [:cjones] [:warhammer] from comment #13)
> Mike, does your patch here get preview working on the Nexus S?
> 
> Inder, is the camera HAL itself actually writing data into our preview
> surface?  Also, can you attach a backtrace from the crash?

cjones: yes, my patch gets the preview working on Nexus S.
Attached patch updated patch (obsolete) — Splinter Review
updated patch, adding a small fix to Inder's and uncommenting the delete[]

This patch works when we compile with --disable-jemalloc and apply the patch in bug 749695
Attachment #611061 - Attachment is obsolete: true
Attachment #614391 - Attachment is obsolete: true
Attachment #618517 - Attachment is obsolete: true
(In reply to Chris Jones [:cjones] [:warhammer] from comment #13)
> Mike, does your patch here get preview working on the Nexus S?
> 
> Inder, is the camera HAL itself actually writing data into our preview
> surface?  Also, can you attach a backtrace from the crash?

cjones: camera HAL writes data into a gralloc'ed buffer which gets copied to another buffer allocated through moz_malloc() before being passed on to preview surface. Seems like Fabrice was able to resolve allocator/deallocator mismatch. I will cleanup the CameraNativeWindow implementation.
Attached patch updated patch (obsolete) — Splinter Review
Better patch, that now works without compiling with --disable-jemalloc

The trick is to redefine the new and delete operators for the 2 classes that were leading to crashes.
Attachment #619105 - Attachment is obsolete: true
Sorry, I was testing with the wrong build :(
Still crashing without --disable-jemalloc, but I've not gave up yet
If there's no better way, you can call __real_malloc or whatever the name is that --wrap generates.
Even better: we should build the jemalloc lib directly into the b2g executable, so that malloc/free always resolve to jemalloc and we can stop bothering with this hackery! ;)

(We do that on desktop linux-x11, but can't do that on android.)
(In reply to Chris Jones [:cjones] [:warhammer] from comment #20)
> Even better: we should build the jemalloc lib directly into the b2g
> executable, so that malloc/free always resolve to jemalloc and we can stop
> bothering with this hackery! ;)
> 
> (We do that on desktop linux-x11, but can't do that on android.)

That doesn't work. The bionic linker is too dumb.
We own the whole stack -- we could modify the bionic source to call jemalloc if that's the only way to make it work, right?
It would be a tiny patch to make the bionic linker lookup symbol in the executable like what happens on desktop linux. However, I'm wondering if it's not better to leave it this way, so that such issues are "shared" with mobile, which *has* to live with the two allocators.
I think we should fix this. We will use more mobile HAL APIs than mobile fennec (camera, gralloc, etc).
There's no reason to force b2g to have two heaps just because fennec is stuck with two.  I agree we should share technical solutions where there aren't trade-offs that need to be made.

I'm not sure I understand comment 25 ... does the jemalloc.a -> b2g trick work or not?  Should be really simple to test, a one-liner somewhere in http://mxr.mozilla.org/mozilla-central/source/b2g/app/Makefile.in (in theory).
(In reply to Chris Jones [:cjones] [:warhammer] from comment #26)
> I'm not sure I understand comment 25 ... does the jemalloc.a -> b2g trick
> work or not?  Should be really simple to test, a one-liner somewhere in
> http://mxr.mozilla.org/mozilla-central/source/b2g/app/Makefile.in (in
> theory).

Discussed this on IRC this morning -

[08:00:38]  <mwu> glandium: so I took a look at the main binary symbol thing
[08:00:48]  <mwu> it looks like it searchs those symbols last
[08:01:02]  <glandium> err that's retarded
[08:01:12]  <mwu> can you double check?
[08:01:34]  <mwu> also I'm thinking LD_PRELOAD might be another option here
[08:01:45]  <mwu> we have control over how b2g is started
[08:04:47]  <glandium> mwu: so, yeah, the executables symbols are searched last. And yes, you could use LD_PRELOAD
BIONIC!!!111!!!

(To be fair though, I think the trick we use on linux-x11 is not well-defined behavior.)

Are we using the custom linker for b2g?  I don't recall.
(In reply to Chris Jones [:cjones] [:warhammer] from comment #28)
> BIONIC!!!111!!!
> 
> (To be fair though, I think the trick we use on linux-x11 is not
> well-defined behavior.)
> 
> Are we using the custom linker for b2g?  I don't recall.

Nope. Just the bionic linker.
OK.  Then LD_PRELOAD for gonk and turning off --wrap sounds like a good solution to me.
See CameraParameters.h for the meaning of the different parameters, and CameraParameters.cpp for the string values to pass to the new API.

The associated camera.js patch exercises these.

This patch contains a reference-counting bug: specifically, if takePicture() or autofocus() is called, closing the camera never releases the GonkCamera object, which means the preview stream is never closed, and eventually the device runs out of memory.

If neither autofocus() nor takePicture() are triggered, closing the camera will allow the GC to delete GonkCamera and close the camera hardware.

In both cases, logging in GonkCamera::ReceiveFrame() says the GonkCamera refcnt is 2.
To trigger autofocus, tap anywhere on the preview image.  A _manuallyFocused variable is supposed to track whether or not such a manual focus has taken place, so that it doesn't need to be done again when the shutter button is pressed, but it doesn't work.

See notes accompanying the most recent patch re: camera shutdown problems.

This is the complete camera.js, not a patch.
Attachment #624938 - Attachment is obsolete: true
Compared to the previous version of this file, this one modifies the Camera.pause() method, and adds an window.onunload event handler; both changes set Camera.viewfinder.src = '', which effectively kills the video stream and prevents the crash.

This is, of course, not a solution, but it will do for now.

(This change also triggers a series of "Invalid URI. Load of media resource  failed." JavaScript warnings in the console log.  Not sure what an appropriate ~empty~ value would be for a stream src; 'about:blank' triggered MIME-type errors.)
Set viewfinder.src = '' in beforeunload instead of unload, to make sure we hit the right window.
Attachment #625105 - Attachment is obsolete: true
The video stream (STILL) doesn't shutdown if you call takePicture(), but does if you just call autofocus().

This includes a call to setTimeout() (which uses a setParameter() hack) to restart the preview after 2 seconds post-shutter.
Attachment #625130 - Attachment is obsolete: true
Also includes a hack to restart the preview.  See notes with camera.js.
Attachment #624933 - Attachment is obsolete: true
As before, but replace .src='' with .src=null, which eliminates GeckoConsole errors.
Attachment #625164 - Attachment is obsolete: true
New version of camera.js that properly shuts down the camera preview stream on exit; no more OoM crash.

(Still not ideal, since it depends on JS being correct.  We really need a method to determine stream buffer congestion so we can drop frames.)
Attachment #625169 - Attachment is obsolete: true
Functionally equivalent to attachment 625165 [details] [diff] [review], but with GonkCamera and GonkCameraHardware broken out into their own files, easier to read.
Attachment #619144 - Attachment is obsolete: true
Attachment #625165 - Attachment is obsolete: true
Attached file Latest replacement camera.js (obsolete) —
camera.js now requires a .play() call on the viewfinder to get the media stream started.

This is brittle, since the camera started streaming preview frames before this call, consuming memory.
Attachment #626119 - Attachment is obsolete: true
Attached file camera.js, now with picture saving! (obsolete) —
Pictures are saved to /data/pictures with filenames:
img_YYYYMMDD-HHMMSS.jpg

The code is very simple--if you manage to take pictures very quickly, you'll likely clobber earlier ones.
Attachment #628182 - Attachment is obsolete: true
blocking-basecamp: --- → ?
blocking-kilimanjaro: --- → ?
I've just tested it by applying by hand the patch to my up-to-date clone of B2G. It works well, however I get huge lag when the camera gets into zones with high luminosity, with a Nexus S. Is it a known bug ?
And it seems to be correlated to a flood in logcat:
E/CameraHardwareSec(  398): int android::CameraHardwareSec::previewThread(): could not obtain gralloc buffer
E/CameraHardwareSec(  398): int android::CameraHardwareSec::previewThread(): could not obtain gralloc buffer
E/CameraHardwareSec(  398): int android::CameraHardwareSec::previewThread(): could not obtain gralloc buffer
This is not related to the gralloc message. I'm still surprised that you get lag - we had a lower framerate than expected and high CPU usage, but no lag.

Hopefully the current implementation performance issues will be solved by bug 757341.
Blocking on a decision at the very least.
blocking-basecamp: ? → +
blocking-kilimanjaro: ? → +
I can confirm that the Nexus S camera has trouble with brightly lit scenes, but I haven't had a chance to dig into it yet.  Alexandre: can you please file a separate bug report?
blocking-basecamp: + → ---
blocking-kilimanjaro: + → ---
Mike, does your comment mean that the issue reported in this bug is specific to the Nexus S and is not generally a problem on ICS?
Done, I filed bug 761521 for this.
(In reply to Lawrence Mandel [:lmandel] from comment #48)
> Mike, does your comment mean that the issue reported in this bug is specific
> to the Nexus S and is not generally a problem on ICS?

Lawrence, it just means I haven't had a chance to play with any other devices.
(In reply to Mike Habicher [:mikeh] from comment #50)
> (In reply to Lawrence Mandel [:lmandel] from comment #48)
> > Mike, does your comment mean that the issue reported in this bug is specific
> > to the Nexus S and is not generally a problem on ICS?
> 
> Lawrence, it just means I haven't had a chance to play with any other
> devices.

At face value it seems to me that the camera not functioning correctly is a show stopper. Can you comment further on why this issue doesn't block basecamp or k9o?
Comment on attachment 631543 [details] [diff] [review]
Rebased on latest b2g gecko rev

git screwed this up, ignore.
Attachment #631543 - Attachment is obsolete: true
Attachment #633264 - Attachment description: WIP - new CameraControl API (doesn' → WIP - new CameraControl API (doesn't work)
blocking-basecamp: --- → ?
We upgraded lower level pieces and broke camera support, needs to be fixed.
blocking-basecamp: ? → +
blocking-kilimanjaro: --- → +
Attached patch WIP - new CameraControl API (obsolete) — Splinter Review
Almost all of the CameraControl features have been implemented.  This must be used with the new camera.js

To do:
- implement camera parameter getters (e.g. focus mode, flash mode, etc.)

Bugs:
- after taking a picture, the viewfinder doesn't resume; there are a lot of stream/page lifecycle issues to resolve.
Attachment #633264 - Attachment is obsolete: true
Attached file Part 0: camera.js (obsolete) —
For use with the new CameraControl API - as noted on that attachment, numerous lifecycle issues remain.
Attachment #635480 - Attachment description: WIP - new camera.js → Part 0: camera.js
Attachment #635789 - Flags: review?(jonas)
Attachment #635794 - Flags: review?(jst)
Comment on attachment 635789 [details] [diff] [review]
Part 1: new CameraControl IDL

Here's a couple of drive-by review comments on this API:

- In nsICameraPictureOptions:

+    /* the rotation of the image in degrees, from 0 to 270 in
+       steps of 90; this doesn't affect the image, only the
+       rotation recorded in the image header.*/
+    attribute long      rotation;

Why limit this to steps of 90 in the API? Sure, implementations will likely do that, but why not leave that up to the implementation and let the API be more flexible than what's normal today?

+    /* an object containing any or all of 'latitude', 'longitude',
+       'elevation', and 'timestamp', used to record when and where
+       the image was taken.  e.g.
+        {
+            latitude:  43.647118,
+            longitude: -79.3943,
+            elevation: 500
+            // timestamp not specified, in this case
+        }
+
+        can be null in the case where position information isn't
+        available/desired.
+
+        'elevation' is in metres; 'timestamp' is UTC, in seconds from
+        January 1, 1970.
+    */
+    attribute jsval     position;

Why not use nsIDOMGeoPositionCoords here? It's already defined, does what you need here and more (which may not be relevant in current implementations, but again, why limit the API?).
(In reply to Johnny Stenback (:jst, jst@mozilla.com) from comment #63)
> Comment on attachment 635789 [details] [diff] [review]
> Part 1: new CameraControl IDL
> 
> Here's a couple of drive-by review comments on this API:
> 
> - In nsICameraPictureOptions:
> 
> +    /* the rotation of the image in degrees, from 0 to 270 in
> +       steps of 90; this doesn't affect the image, only the
> +       rotation recorded in the image header.*/
> +    attribute long      rotation;
> 
> Why limit this to steps of 90 in the API? Sure, implementations will likely
> do that, but why not leave that up to the implementation and let the API be
> more flexible than what's normal today?

Sure, how about: "the rotation of the image recorded in the image header, in degrees, from 0 to 359; values outside this range will be mod-360-ed, and the implementation may round the values to the nearest supported rotation."

> +    /* an object containing any or all of 'latitude', 'longitude',
> +       'elevation', and 'timestamp', used to record when and where
> +       the image was taken.  e.g.
> +        {
> +            latitude:  43.647118,
> +            longitude: -79.3943,
> +            elevation: 500
> +            // timestamp not specified, in this case
> +        }
> +
> +        can be null in the case where position information isn't
> +        available/desired.
> +
> +        'elevation' is in metres; 'timestamp' is UTC, in seconds from
> +        January 1, 1970.
> +    */
> +    attribute jsval     position;
> 
> Why not use nsIDOMGeoPositionCoords here? It's already defined, does what
> you need here and more (which may not be relevant in current
> implementations, but again, why limit the API?).

I'm happy to use nsIDOMGeoPositionCoords, though unlike the above API, it wouldn't allow for missing data.  I'm not sure if that's a real concern, but EXIF certainly allows it.

One new question: when should I be namings things nsI* vs. nsIDOM* ?  Thanks!
(In reply to Mike Habicher [:mikeh] from comment #64)
> Sure, how about: "the rotation of the image recorded in the image header, in
> degrees, from 0 to 359; values outside this range will be mod-360-ed, and
> the implementation may round the values to the nearest supported rotation."

Yeah, that sounds good to me... Another thought that came to mind, should this be a float? Nothing about degrees is integer per se... I can go either way on this though...

> I'm happy to use nsIDOMGeoPositionCoords, though unlike the above API, it
> wouldn't allow for missing data.  I'm not sure if that's a real concern, but
> EXIF certainly allows it.

Good question. It also doesn't actually contain a timestamp, so that would need to be exposed outside of the position field here, which I think makes sense. Maybe this could be defined to return NaN for values that are not available?

> One new question: when should I be namings things nsI* vs. nsIDOM* ?  Thanks!

The traditional wisdom said to use nsIDOM* for interfaces that are exposed to the web, but we don't strictly follow that convention any more, so it's largely up to you. Interface names should be internal to Mozilla anyways, prefixes are cut off as much as we can, and the new DOM bindings are moving away from prefixes altogether.
Attached file Part 0: camera.js (obsolete) —
A new version of camera.js that reverts to use the sledgehammer of restarting the camera to resume the preview.

This also removes backwards compatability with the old mozMedia stuff--we don't need it anymore.

This code is mostly for demo purposes.  Tapping on the 'gallery' button currently cycles through the colour effects mode; tapping on the view-finder triggers autofocus (though it doesn't enable any specific focus region); tapping on the shutter button will take a picture and save it.
Attachment #635480 - Attachment is obsolete: true
Break up the CameraPreview class into that and a CameraPreviewListener, which handles media stream status changes.

There is currently a memory leak because of a reference cycle: CameraPreview creates a new CameraPreviewListener, which has a reference to the CameraPreview object; and passes it to mInput->AddListener(), which holds a reference as well.  So these objects are not garbage-collected yet.

Need to add support for cyclic collection.
Attachment #635791 - Attachment is obsolete: true
Attached patch WIP - new CameraControl API (obsolete) — Splinter Review
Includes the new CameraPreview changes and a needed change to CameraCommon.h, that I will break out into a more specific patch tomorrow.  For now, you can use this on all ICS devices.
Attachment #635479 - Attachment is obsolete: true
Attached patch WIP - new CameraControl API (obsolete) — Splinter Review
Incremental patch that includes files missing from last night's diff.  Apologies.
Attachment #636936 - Attachment is obsolete: true
Attached patch WIP - new CameraControl API (obsolete) — Splinter Review
This has the patches in the correct order.  Silly git, mea culpa.

Apply:
patch -p1 < 740997_inc2.patch

Generated by:
git log --author=mikeh --oneline --reverse | awk '{print $1;}'
and:
git show aa9c36f 5831171 37086fc 268f97f 9831cdd fdc0f89 bce5832 63628c6 1a3ee04 879084f 64e74d7 e2dcf65 25ba990 dd4fa0c 48dac65 bf7217d 0fab6c8 9c7ce62 32f8010 d9f2b10 c3f365b 1d2c77a b40d91b a9615bb c3c0c54 77e59b8 c35f022 > 740997_inc2.patch
Attachment #637047 - Attachment is obsolete: true
Attached patch WIP - new CameraControl API (obsolete) — Splinter Review
This has the patches in the correct order, and with a previously missing small change.  *sigh*... no more 2am patches....

Apply:
patch -p1 < 740997_inc2.patch

Generated by:
git log --author=mikeh --oneline --reverse | awk '{print $1;}'
and:
git show aa9c36f 5831171 37086fc 268f97f 9831cdd fdc0f89 bce5832 63628c6 1a3ee04 879084f 64e74d7 e2dcf65 25ba990 dd4fa0c 48dac65 bf7217d 0fab6c8 9c7ce62 32f8010 d9f2b10 c3f365b 1d2c77a b40d91b a9615bb c3c0c54 77e59b8 c35f022 eda12dd > 740997_inc3.patch

If you don't want to repatch and rebuild, just open dom/camera/CameraPreview.h and comment out NotifyConsumptionChanged().
Attachment #637048 - Attachment is obsolete: true
Attached patch WIP - new CameraControl API (obsolete) — Splinter Review
New, smaller diff of the CameraControl API.  In addition to all previous changes, this one also includes:
- mutex protection on CameraControl's CameraParameters database
- support for retrieving the three focus distances (although the results returned on Akami don't really correlate with... well... reality)
Attachment #637059 - Attachment is obsolete: true
Attached patch Part 1: new CameraControl IDL (obsolete) — Splinter Review
Updated to revise description of 'rotation' attribute as per jst's feedback; left this as a 'long'--a 'double' feels like overkill here.

Left 'position' as-is (for now).
Attachment #635789 - Attachment is obsolete: true
Attachment #635789 - Flags: review?(jonas)
Attachment #637879 - Flags: review?(jst)
Attachment #637879 - Flags: review?(jonas)
Comment on attachment 637879 [details] [diff] [review]
Part 1: new CameraControl IDL

Tagging as a patch.
Attachment #637879 - Attachment is patch: true
Comment on attachment 635794 [details] [diff] [review]
Part 3: new DOM framework

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

::: dom/camera/DOMCameraManager.cpp
@@ +128,5 @@
> +      if (JSVAL_IS_STRING(v)) {
> +        const char* camera = JS_EncodeString(cx, JSVAL_TO_STRING(v));
> +        if (camera) {
> +          if (strcmp(camera, "back") == 0) {
> +            cameraId = 1;

This is causing a crash here, where the back camera is 0, not 1.
Attached patch WIP - new CameraControl API (obsolete) — Splinter Review
Includes a fix for the single-camera Otoro, and adds better handling of invalid cameras.
Attachment #627788 - Attachment is obsolete: true
Attachment #628949 - Attachment is obsolete: true
Attachment #631553 - Attachment is obsolete: true
Attachment #637127 - Attachment is obsolete: true
Attached patch WIP - new CameraControl API (obsolete) — Splinter Review
Add support for exposure compensation.  setExposureCompensation() can be called with a float value that will configure the camera; it can also be called with no parameters or with null, which will return the camera to automatic exposure compensation mode.
Attachment #641190 - Attachment is obsolete: true
Mike, what's blocking here other than review from sicking and jst?  How's the implementation looking?

sicking/jst, review ping :).
(In reply to Chris Jones [:cjones] [:warhammer] from comment #78)
> Mike, what's blocking here other than review from sicking and jst?  How's
> the implementation looking?
> 
> sicking/jst, review ping :).

Hi Chris, the implementation is just about done for still photos.  I need to finish up some OnNavigation handling, and then integrate the changes for video recording.  After that it should be all bug-fixen.

The IDL should be frozen now.
Attached patch Part 1: new CameraControl IDL (obsolete) — Splinter Review
Changed setExposureCompensation() to take an optional in jsval, so that it can be called with null or void to enable automatic exposure compensation.  (Turns out a missing optional double is set to 0, which is a valid value.)
Attachment #636923 - Attachment is obsolete: true
Attachment #637879 - Attachment is obsolete: true
Attachment #637879 - Flags: review?(jst)
Attachment #637879 - Flags: review?(jonas)
Attachment #642566 - Flags: review?(jst)
Attachment #642566 - Flags: review?(jonas)
Attached patch Part 3: new DOM framework (obsolete) — Splinter Review
Latest version of the DOM code and build framework (makefiles, etc.)
Attachment #635794 - Attachment is obsolete: true
Attachment #635794 - Flags: review?(jst)
Attachment #642617 - Flags: review?(jst)
Attachment #636931 - Attachment is obsolete: true
Attachment #642618 - Flags: review?(roc)
Attached patch Part 4: new Gonk-ICS support (obsolete) — Splinter Review
Attachment #635795 - Attachment is obsolete: true
Attachment #642620 - Flags: review?(fabrice)
Attached patch WIP - new CameraControl API (obsolete) — Splinter Review
Attachment #641564 - Attachment is obsolete: true
Comment on attachment 642617 [details] [diff] [review]
Part 3: new DOM framework

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

::: dom/camera/CameraCommon.h
@@ +18,5 @@
> +#define END_CAMERA_NAMESPACE \
> +  } /* namespace camera */ } /* namespace dom */ } /* namespace mozilla */
> +
> +#define USING_CAMERA_NAMESPACE \
> +  using namespace mozilla::dom::camera;

Using macros to handle namespacing means you're using too many namespaces :-).

Most of your classes have Camera in the name. I suggest that "namespace mozilla;" would be perfectly adequate here.
Comment on attachment 642620 [details] [diff] [review]
Part 4: new Gonk-ICS support

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

Here's some random driveby comments.

Can we remove the deprecated_* files?

CameraNativeWindow.cpp needs a license.

::: dom/camera/GonkCameraControl.h
@@ +25,5 @@
> +  friend class SetParameterTask;
> +  friend class GetParameterTask;
> +  friend class PushParametersTask;
> +  friend class PullParametersTask;
> + */

Remove?

@@ +54,5 @@
> +  nsresult DoPullParameters(PullParametersTask *aPullParameters);
> +
> +  PRUint32                        mHwHandle;
> +  double                          mExpsoureCompensationMin;
> +  double                          mExpsoureCompensationStep;

exposure spelled wrong

::: dom/camera/GonkCameraControl.cpp
@@ +358,5 @@
> +void
> +nsGonkCameraControl::SetParameter(PRUint32 aKey, const char *aValue)
> +{
> +  const char *key = getKeyText(aKey);
> +  if (key) {

if (!key)
  return;

Here and in the next two functions.

::: dom/camera/GonkCameraHwMgr.cpp
@@ +38,5 @@
> +}
> +#endif
> +
> +
> +#if GIHM_STUB_NATIVEWINDOW

Is there ever a case where we'd prefer the stub window over the native window?

@@ +141,5 @@
> +    return;
> +  }
> +
> +  GonkCamera* camera = hw->mTarget;
> +  if (camera) {

if (!camera)
  return;

@@ +226,5 @@
> +{
> +  GonkCameraHardware* hw = getCameraHardware(aHwHandle);
> +  DOM_CAMERA_LOGI("%s: aHwHandle = %d, hw = %p (sHwHandle = %d)\n", __func__, aHwHandle, (void*)hw, sHwHandle);
> +
> +  if (hw) {

if (!hw)
  return;

@@ +452,5 @@
> +  }
> +}
> +
> +void
> +// GonkCameraHardware::GonkCameraHardware::doCameraHardwareStopPreview(PRUint32 aHwHandle)

remove
Comment on attachment 642618 [details] [diff] [review]
Part 2: new CameraPreview (MediaStream-specific code)

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

::: dom/camera/GonkCameraPreview.cpp
@@ +32,5 @@
> +  nsRefPtr<Image> image = mImageContainer->CreateImage(&format, 1);
> +  image->AddRef();
> +  PlanarYCbCrImage *videoImage = static_cast<PlanarYCbCrImage*>(image.get());
> +
> +  if (!mIs420p) {

If it's not 420p, what is it? Instead of a bool it would be nicer to have an enum that describes what we actually do have.

@@ +35,5 @@
> +
> +  if (!mIs420p) {
> +    uint8_t* y = aData;
> +    uint32_t yN = mWidth * mHeight;
> +    uint32_t uvN = yN / 4;

What if yN is not divisible by 4? Should we assert that it is?

@@ +47,5 @@
> +    uint32_t u1;
> +    uint32_t v1;
> +
> +    uint32_t src0;
> +    uint32_t src1;

Move these declarations down to where the variables are initialized.

@@ +49,5 @@
> +
> +    uint32_t src0;
> +    uint32_t src1;
> +
> +    uvN /= 8;

So uvN used to count bytes and now it counts ... 8-byte words? Better to use a new variable name.

@@ +66,5 @@
> +
> +      u1 = ( src0 & 0xFF00UL ) >> 8 | ( src0 & 0xFF000000UL ) >> 16;
> +      u1 |= ( src1 & 0xFF00UL ) << 8 | ( src1 & 0xFF000000UL );
> +      v1 = ( src0 & 0xFFUL ) | ( src0 & 0xFF0000UL ) >> 8;
> +      v1 |= ( src1 & 0xFFUL ) << 16 | ( src1 & 0xFF0000UL ) << 8;

Use inline functions or macros (or preferably, just one inline function or macro) so we're not repeating this code. Also then you can give it a meaningful name to make this code more readable.

@@ +87,5 @@
> +  data.mYStride = mWidth * lumaBpp / 8.0;
> +  data.mCbCrStride = mWidth * chromaBpp / 8.0;
> +  data.mCbChannel = aData + mHeight * data.mYStride;
> +  data.mCrChannel = data.mCbChannel + mHeight * data.mCbCrStride / 2;
> +  data.mCbCrSize = gfxIntSize(mWidth / 2, mHeight / 2);

Add some assertions to check that these divisions by 8 and 2 are dividing things that are actually divisible by 8 and 2, and add comments to explain how you can be sure of that.

@@ +110,5 @@
> +{
> +  /*
> +  mInput->EndTrack(TRACK_VIDEO);
> +  mInput->Finish();
> +  */

Remove commented-out code!

@@ +117,5 @@
> +
> +  GonkCameraHardware::setCameraHardwarePreviewSize(mHwHandle, mWidth, mHeight);
> +  GonkCameraHardware::getCameraHardwarePreviewSize(mHwHandle, &mWidth, &mHeight);
> +  SetFrameRate(GonkCameraHardware::getCameraHardwareFps(mHwHandle));
> +  if (GonkCameraHardware::doCameraHardwareStartPreview(mHwHandle) == OK) {

Why don't these methods start with a capital letter?

@@ +118,5 @@
> +  GonkCameraHardware::setCameraHardwarePreviewSize(mHwHandle, mWidth, mHeight);
> +  GonkCameraHardware::getCameraHardwarePreviewSize(mHwHandle, &mWidth, &mHeight);
> +  SetFrameRate(GonkCameraHardware::getCameraHardwareFps(mHwHandle));
> +  if (GonkCameraHardware::doCameraHardwareStartPreview(mHwHandle) == OK) {
> +    // mState = HW_STATE_PREVIEW;

remove commented-out code

@@ +130,5 @@
> +GonkCameraPreview::Stop()
> +{
> +  DOM_CAMERA_LOGI("%s:%d : this=%p\n", __func__, __LINE__, this);
> +
> +  GonkCameraHardware::doCameraHardwareStopPreview(mHwHandle);

Something should do an EndTrack and Finish. Here?

::: dom/camera/CameraPreview.h
@@ +17,5 @@
> +
> +BEGIN_CAMERA_NAMESPACE
> +
> +static const TrackID TRACK_AUDIO = 1;
> +static const TrackID TRACK_VIDEO = 2;

Use an enum to place these in the CameraPreview class. Do you actually need an audio track?
MikeH, Here is a list of files that needs to be updated for Apache2 license:
CameraControl.cpp
CameraControl.h
GonkCameraControl.cpp
GonkCameraHwMgr.cpp
GonkCameraHwMgr.h
GonkCameraPreview.cpp
Comment on attachment 642617 [details] [diff] [review]
Part 3: new DOM framework

Are things like previewSizes returning a new object every time on purpose?  If they had a way to return an arraylike object that the caller could not mutate (and they could also not mutate) but they could return the same exact object each time, would that be good or bad?

I ask because in WebIDL an attribute getter cannot return a new array every time, purposefully (because web developers don't really expect an attribute get to create a new object each time).
Comment on attachment 642617 [details] [diff] [review]
Part 3: new DOM framework

This generally looks good! I found a number of mostly minor things while reading through this, and I'm saying r- only because I want to look this over once more after the following stuff has been fixed. And we need to sort out the issue that bz pointed out as well.

Here's my findings:

- In setHelper() (and friends):

+  const char *v = ToNewCString(aValue);
+  if (v) {
+    aCameraContol->SetParameter(aKey, v);
+    nsMemory::Free(const_cast<char*>(v));
+    return NS_OK;
+  } else {
+    return NS_ERROR_OUT_OF_MEMORY;
+  }

Remove the else after return, maybe simply return NS_ERROR_OUT_OF_MEMORY if (!v) here, and end with the actual logic in this function. This pattern appears in lots of these helpers, please change them all.

And in this particular version, any reason to not use NS_ConvertUTF16toUTF8(aValue).get()? That'll be faster for short strings, work with non-ascii characters, and be safe for long strings as well. And less code.

+static nsresult
+setHelper(nsCameraControl *aCameraContol, PRUint32 aKey, double aValue)
+{
+  aCameraContol->SetParameter(aKey, aValue);
+  return NS_OK;
+}
+
+static nsresult
+getHelper(nsCameraControl *aCameraControl, PRUint32 aKey, double *aValue)
+{
+  if (aValue) {
+    *aValue = aCameraControl->GetParameterDouble(aKey);
+    return NS_OK;
+  } else {
+    return NS_ERROR_UNEXPECTED;
+  }
+}

From the looks of things, aValue always comes from the bindings layer (i.e. XPConnect). If that's the case, you can simply MOZ_ASSERT() that it's non-null, and don't bother checking. Which begs the question, do these helpers really buy you anything? I.e. they'll both just call a function and return. Seems the callers can do that just as well. If you still want them, maybe just inline them so we don't pay the size/cost of another function here.

- In setHelper(nsCameraControl *aCameraContol, PRUint32 aKey, const JS::Value & aValue, JSContext *cx):

+      if (JS_GetArrayLength(cx, regions, &length)) {
+        DOM_CAMERA_LOGI("%s:%d : got %d regions\n", __func__, __LINE__, length);
+        parsedRegions = new nsCameraControl::CameraRegion[length];

Should we sanity check length here? What if I pass in a sparse array with a length of a billion or what not? We'll oom and die safely from the looks of things, but seems we can handle that better.

+          if (JS_GetElement(cx, regions, i, &v)) {
+            if (v.isObject()) {

Maybe just move the v.isObject() check up into the above if check?

+              /* TODO: move these Gonk-specific values somewhere else */
+              PRInt32 top     = -1000;
+              PRInt32 left    = -1000;
+              PRInt32 bottom  =  1000;
+              PRInt32 right   =  1000;
+              PRUint32 weight =  1;
+
+              if (JS_GetProperty(cx, r, "top", &p)) {
+                if (JSVAL_IS_INT(p)) {
+                  top = JSVAL_TO_INT(p);
+                }
+              }

Maybe write an inline helper to do the above? I.e. something like this:

static PRInt32
getPropertyHelper(JSContext *cx, JSObject *o, char *prop, PRInt32 default)
{
  jsval p;
  if (JS_GetProperty(cx, r, "top", &p) && JSVAL_IS_INT(p)) {
    return JSVAL_TO_INT(p);
  }

  return default;
}

and then just do parsed->mTop = getPropertyHelper(cx, r, "top", -1000)?

- In nsCameraControl::GetCapabilities():

+  nsCOMPtr<nsICameraCapabilities> capabilities = mCapabilities;
+
+  if (!capabilities) {
+    capabilities = new nsCameraCapabilities(this);
+    if (!capabilities) {
+      return NS_ERROR_OUT_OF_MEMORY;
+    }
+  }
+
+  capabilities.forget(aCapabilities);
+  return NS_OK;

Looks like this meant to set mCapabilities inside the outermost if check. In fact, w/o that, mCapabilities is effectively unused.

- In nsCameraControl::SetExposureCompensation():

+  if (JSVAL_IS_DOUBLE(aCompensation)) {
+    double compensation = JSVAL_TO_DOUBLE(aCompensation);
+    return setHelper(this, CAMERA_PARAM_EXPOSURECOMPENSATION, compensation);
+  } else if(JSVAL_IS_INT(aCompensation)) {

Remove this and other else-after-return's.

- In nsCameraControl::StartRecording():

+    if (JS_GetProperty(cx, options, "width", &v)) {
+      if (JSVAL_IS_INT(v)) {
+        width = JSVAL_TO_INT(v);
+      }
+    }

Maybe use the helper from earlier in this comment? Many more of the same below...

- In dom/camera/DOMCameraManager.h:

+class nsDOMCameraManager : public nsIDOMCameraManager
[...]
+  static nsRefPtr<nsDOMCameraManager> sCameraManager;

This looks unused.

- In class GetPreviewStreamTask:

+  PRUint32 mWidth;
+  PRUint32 mHeight;
+  nsCOMPtr<nsCameraControl> mCameraControl;
+  nsCOMPtr<nsICameraPreviewStreamCallback> mOnSuccessCb;
+  nsCOMPtr<nsICameraErrorCallback> mOnErrorCb;

Probably want at least the last two of those to be protected.

- In class AutoFocusTask:

+  nsCOMPtr<nsCameraControl> mCameraControl;
+  nsCOMPtr<nsICameraAutoFocusCallback> mOnSuccessCb;
+  nsCOMPtr<nsICameraErrorCallback> mOnErrorCb;

Same here and in TakePictureTask and StartRecordingTask as well.

- In class TakePictureTask:

+  double mLatitude;
+  bool mLatitudeSet;
+  double mLongitude;
+  bool mLongitudeSet;
+  double mAltitude;
+  bool mAltitudeSet;
+  double mTimestamp;
+  bool mTimestampSet;

You could save yourself several bytes in the size of this struct if you re-order those to have the bools next to other bools.

- In dom/camera/CameraCapabilities.cpp:

+#define CHECK_CAMERA_PTR(c)         \
+  if (!c) {                         \
+    return NS_ERROR_NOT_AVAILABLE;  \
+  }

Why not use NS_ENSURE_TRUE(c, NS_ERROR_NOT_AVAILABLE) instead of yet another macro that hides a return?

- In nsCameraCapabilities::~nsCameraCapabilities()

+  // DOM_CAMERA_LOGI("%s:%d - |mCamera = nsnull| removed\n", __func__, __LINE__);

Remove this commented out line?

+  mCamera = nsnull;

The dtor will do this for you... Plus, it doesn't matter here, this object is dead after this destructor returns, and no code should see its members after this.

- In Navigator::Invalidate():

+  if (mCameraManager) {
+    mCameraManager = nsnull;
+  }

No need to check, just assign null.

- In nsDOMCameraManager::OnNavigation(PRUint64 aWindowId):

+  /* TODO: implement -- see getUserMedia() implementation */

This seems like something we want to do before we land this IMO.

- In nsDOMCameraManager::Create(PRUint64 aWindowId, nsDOMCameraManager * *aMozCameras):

+  /* TODO: check for permissions here to access cameras */

Same here.
Attachment #642617 - Flags: review?(jst) → review-
This builds but is not yet tested.  Parking it here for now because I haven't had the best luck with laptop hard drives....
No longer blocks: 774552
Attachment #642620 - Flags: review?(fabrice) → review?(gal)
Comment on attachment 642620 [details] [diff] [review]
Part 4: new Gonk-ICS support

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

This patch needs a bunch of polish. Lets iterate over it.

::: dom/camera/FallbackCameraManager.cpp
@@ +10,5 @@
> +*/
> +
> +/* [implicit_jscontext] void getCamera ([optional] in jsval aOptions, in nsICameraGetCameraCallback onSuccess, [optional] in nsICameraErrorCallback onError); */
> +NS_IMETHODIMP
> +nsDOMCameraManager::GetCamera(const JS::Value & aOptions, nsICameraGetCameraCallback *onSuccess, nsICameraErrorCallback *onError, JSContext* cx)

We don't seem to have a consistent convention for * in dom/ but at least make it consistent per file, so "JSContext *".

::: dom/camera/GonkCameraHwMgr.h
@@ +37,5 @@
> +
> +public:
> +  static void                   releaseCameraHardwareHandle(PRUint32 aHwHandle);
> +  static PRUint32               getCameraHardwareHandle(GonkCamera* aTarget, PRUint32 aCamera);
> +  static PRUint32               getCameraHardwareFps(PRUint32 aHwHandle);

You are inside a class. I don't think we have to namespace method names. getFps() getHandle() should be fine.

@@ +40,5 @@
> +  static PRUint32               getCameraHardwareHandle(GonkCamera* aTarget, PRUint32 aCamera);
> +  static PRUint32               getCameraHardwareFps(PRUint32 aHwHandle);
> +  static void                   getCameraHardwarePreviewSize(PRUint32 aHwHandle, PRUint32* aWidth, PRUint32* aHeight);
> +  static void                   setCameraHardwarePreviewSize(PRUint32 aHwHandle, PRUint32 aWidth, PRUint32 aHeight);
> +  static int                    doCameraHardwareAutoFocus(PRUint32 aHwHandle);

autoFocus()
cancelAutoFocus()
takePicture()
cancelTakePicture()
startPreview()
stopPreview()
setParameters()
getParameters()

::: dom/camera/GonkCameraControl.h
@@ +44,5 @@
> +
> +  void ReceiveFrame(PRUint8 *aData, PRUint32 aLength);
> +
> +protected:
> +  nsresult DoGetPreviewStream(GetPreviewStreamTask *aGetPreviewStream);

Dito here. Sane naming please.

@@ +61,5 @@
> +  android::CameraParameters       mParams;
> +};
> +
> +/* camera driver callbacks */
> +void GonkCameraReceiveImage(nsGonkCameraControl* gc, PRUint8* aData, PRUint32 aLength);

You are in the camera namespace. I don't think you need GonkCamera here either.

::: dom/camera/GonkCameraControl.cpp
@@ +56,5 @@
> +  }
> +}
> +
> +/*
> +  Gonk-specific CameraControl implementation.

Can you use // for these kind of comments. /* */ on separate lines seems really wasteful. Sorry for the visual nits.

@@ +171,5 @@
> +        ++index;
> +        /* intentional fallthrough */
> +
> +      case CAMERA_PARAM_FOCUSDISTANCEOPTIMUM:
> +        ++index;

This is really magical. Calling a helper function is almost always preferred.

@@ +176,5 @@
> +        /* intentional fallthrough */
> +
> +      case CAMERA_PARAM_FOCUSDISTANCENEAR:
> +        s = mParams.get(key);
> +        // DOM_CAMERA_LOGI("key='%s' --> value='%s'\n", key, s);

Remove.

@@ +180,5 @@
> +        // DOM_CAMERA_LOGI("key='%s' --> value='%s'\n", key, s);
> +        if (!s) {
> +          return 0.0;
> +        }
> +        while(index) {

while_(

@@ +188,5 @@
> +          }
> +          ++s;
> +          --index;
> +        }
> +        val = strtod(s, nsnull);

return strtod

@@ +198,5 @@
> +  } else {
> +    if (aKey == CAMERA_PARAM_ZOOM) {
> +      /* return 1x when zooming is not supported */
> +      return 1.0;
> +    } else {

else not needed.

@@ +210,5 @@
> +{
> +  const char* key = getKeyText(aKey);
> +  RwAutoLockRead lock(mRwLock);
> +
> +  if (!key) {

No need to lock for this. Move lock down.

@@ +302,5 @@
> +          /* intentional fallthrough */
> +
> +        default:
> +          DOM_CAMERA_LOGE("%s:%d : malformed region '%s'\n", __func__, __LINE__, p);
> +          goto GetParameter_error;

can you do this with break? This is extremely ugly.

@@ +339,5 @@
> +    if (NS_IsMainThread()) {
> +      nsCOMPtr<nsIRunnable> pushParametersTask = new PushParametersTask(this);
> +      mCameraThread->Dispatch(pushParametersTask, NS_DISPATCH_NORMAL);
> +    } else {
> +      DoPushParameters(nsnull);

For consistency you could actually always dispatch (even when you are already on the camera thread). That would be less code.

@@ +411,5 @@
> +    char *p = s;
> +    PRUint32 n;
> +
> +    for (PRUint32 i = 0; i < aLength; ++i) {
> +      CameraRegion *r = &aRegions[i];

Please search for AppendPrintf. Using proper XPCOM strings is much safer and cleaner here.

@@ +440,5 @@
> +{
> +  nsCOMPtr<CameraPreview> preview = mPreview;
> +
> +  if (!preview) {
> +    preview = new GonkCameraPreview(mHwHandle, aGetPreviewStream->mWidth, aGetPreviewStream->mHeight);

If !preview new preview

and then dispatch, no need to duplicate

@@ +496,5 @@
> +    mTakePictureOnSuccessCb = nsnull;
> +    nsCOMPtr<nsICameraErrorCallback> ecb = mTakePictureOnErrorCb;
> +    mTakePictureOnErrorCb = nsnull;
> +    if (ecb) {
> +      if (NS_FAILED(NS_DispatchToMainThread(new CameraErrorResult(ecb, NS_LITERAL_STRING("CANCELLED"))))) {

Don't call functions inside a macro.

::: dom/camera/CameraNativeWindow.cpp
@@ +185,5 @@
> +                }
> +            }
> +        }
> +
> +#if 0 //XXX: Not sure if we need to do this check

???

@@ +284,5 @@
> +    } else {
> +        timestamp = mTimestamp;
> +    }
> +
> +    //XXX:

???

@@ +303,5 @@
> +int CameraNativeWindow::lockBuffer(ANativeWindowBuffer* buffer)
> +{
> +    CNW_LOGD("CameraNativeWindow::lockBuffer");
> +    Mutex::Autolock lock(mMutex);
> +    //TODO: Need to implement this.

???

@@ +318,5 @@
> +        CNW_LOGE("cancelBuffer: slot index out of range [0, %d]: %d",
> +                mBufferCount, buf);
> +        return -EINVAL;
> +    } else if (mSlots[buf].mBufferState != BufferSlot::DEQUEUED) {
> +        printf_stderr("cancelBuffer: slot %d is not owned by the client (state=%d)",

NS_WARNING or NS_ASSERTION?

::: dom/camera/GonkCameraManager.cpp
@@ +28,5 @@
> +  if (!a) {
> +    DOM_CAMERA_LOGE("getListOfCameras : Could not create array object");
> +    return NS_ERROR_OUT_OF_MEMORY;
> +  }
> +  if (hw_get_module(CAMERA_HARDWARE_MODULE_ID,

one line

::: dom/camera/deprecated_FallbackImpl.cpp
@@ +50,5 @@
> +    mTimer = nsnull;
> +  }
> +
> +  void init() {
> +    printf_stderr("XxXxX init()\n");

???

@@ +65,5 @@
> +  //
> +  // MediaStreamListener
> +  //
> +  void NotifyBlockingChanged(MediaStreamGraph* aGraph, Blocking aBlocked) {
> +    printf_stderr("XxXxX NotifyBlockingChanged blocked=%d\n", aBlocked);

??? these printfs should go or become proper logging

@@ +145,5 @@
> +
> +    mNumFrames++;
> +    mVideoSegment.AppendFrame(videoImage, 1, gfxIntSize(mWidth, mHeight));
> +    mInput->AppendToTrack(TRACK_VIDEO, &mVideoSegment);
> +    //PRUint32 time = SecondsToMediaTime(1 + mNumFrames / mFps);

???

::: dom/camera/GonkCameraHwMgr.cpp
@@ +213,5 @@
> +}
> +
> +GonkCameraHardware::~GonkCameraHardware()
> +{
> +  DOM_CAMERA_LOGI( "XxXxX %s: this = %p\n", __func__, (void*)this );

clean this up too please

@@ +372,5 @@
> +int
> +GonkCameraHardware::doCameraHardwareTakePicture(PRUint32 aHwHandle)
> +{
> +  GonkCameraHardware* hw = getCameraHardware(aHwHandle);
> +  if (hw) {

if !hw
  return !OK; (is there ERROR or something like that?)
(In reply to Michael Wu [:mwu] from comment #86)
> Comment on attachment 642620 [details] [diff] [review]
> Part 4: new Gonk-ICS support
> 
> Review of attachment 642620 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Here's some random driveby comments.
> 
> Can we remove the deprecated_* files?

Done.

> CameraNativeWindow.cpp needs a license.

It's never had a license, and I have no idea where it came from.  :fabrice might, but he's on vacation until next week.

> ::: dom/camera/GonkCameraHwMgr.cpp
> @@ +38,5 @@
> > +}
> > +#endif
> > +
> > +
> > +#if GIHM_STUB_NATIVEWINDOW
> 
> Is there ever a case where we'd prefer the stub window over the native
> window?

Sometimes, particular if it's giving us trouble on a new platform.  e.g. the Nexus S initially didn't work with native windows enabled, and early porting was done with the stub windows.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #87)
> Comment on attachment 642618 [details] [diff] [review]
> Part 2: new CameraPreview (MediaStream-specific code)
> 
> Review of attachment 642618 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ +130,5 @@
> > +GonkCameraPreview::Stop()
> > +{
> > +  DOM_CAMERA_LOGI("%s:%d : this=%p\n", __func__, __LINE__, this);
> > +
> > +  GonkCameraHardware::doCameraHardwareStopPreview(mHwHandle);
> 
> Something should do an EndTrack and Finish. Here?

I was hoping you could tell me that.  :)

> ::: dom/camera/CameraPreview.h
> @@ +17,5 @@
> > +
> > +BEGIN_CAMERA_NAMESPACE
> > +
> > +static const TrackID TRACK_AUDIO = 1;
> > +static const TrackID TRACK_VIDEO = 2;
> 
> Use an enum to place these in the CameraPreview class. Do you actually need
> an audio track?

Not for the preview, no.
(In reply to Boris Zbarsky (:bz) from comment #89)
> Comment on attachment 642617 [details] [diff] [review]
> Part 3: new DOM framework
> 
> Are things like previewSizes returning a new object every time on purpose? 
> If they had a way to return an arraylike object that the caller could not
> mutate (and they could also not mutate) but they could return the same exact
> object each time, would that be good or bad?

For a particular instance of CameraCapabilities, the data is fixed.  (If a new CameraControl object is acquired, then the CameraCapabilities might change.)

> I ask because in WebIDL an attribute getter cannot return a new array every
> time, purposefully (because web developers don't really expect an attribute
> get to create a new object each time).

Makes sense.  I'll build one instance of the array object and return it on subsequent calls.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #87)
> Comment on attachment 642618 [details] [diff] [review]
> Part 2: new CameraPreview (MediaStream-specific code)
> 
> Review of attachment 642618 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ +35,5 @@
> > +
> > +  if (!mIs420p) {
> > +    uint8_t* y = aData;
> > +    uint32_t yN = mWidth * mHeight;
> > +    uint32_t uvN = yN / 4;
> 
> What if yN is not divisible by 4? Should we assert that it is?

In this particular instance, I don't think we have to worry.  AFAIK, there are no video/screen formats that generate odd dimensions: both width and height are even, and so divisible by 2, so their product will always be divisible by 4.

> @@ +87,5 @@
> > +  data.mYStride = mWidth * lumaBpp / 8.0;
> > +  data.mCbCrStride = mWidth * chromaBpp / 8.0;
> > +  data.mCbChannel = aData + mHeight * data.mYStride;
> > +  data.mCrChannel = data.mCbChannel + mHeight * data.mCbCrStride / 2;
> > +  data.mCbCrSize = gfxIntSize(mWidth / 2, mHeight / 2);
> 
> Add some assertions to check that these divisions by 8 and 2 are dividing
> things that are actually divisible by 8 and 2, and add comments to explain
> how you can be sure of that.

Right now, this is guaranteed by the values of lumaBpp (8) and chromaBpp (4).

This function is executed 30 times per second--I'd rather not add extra code if it's not strictly necessary.  Would some detailed comments suffice?
> I'll build one instance of the array object and return it on subsequent calls.

To be clear, I wasn't advocating for this.

That said, if you also freeze the array object, it might be ok.  Otherwise consumers will be able to modify it under you....
(In reply to Andreas Gal :gal from comment #92)
> Comment on attachment 642620 [details] [diff] [review]
> Part 4: new Gonk-ICS support
> 
> Review of attachment 642620 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This patch needs a bunch of polish. Lets iterate over it.

--- 8< ---

> ::: dom/camera/GonkCameraHwMgr.h
> @@ +37,5 @@
> > +
> > +public:
> > +  static void                   releaseCameraHardwareHandle(PRUint32 aHwHandle);
> > +  static PRUint32               getCameraHardwareHandle(GonkCamera* aTarget, PRUint32 aCamera);
> > +  static PRUint32               getCameraHardwareFps(PRUint32 aHwHandle);
> 
> You are inside a class. I don't think we have to namespace method names.
> getFps() getHandle() should be fine.

The naming convention was leftover from an earlier iteration of the code.  I'm going to shorten all of these as per yours and :roc's feedback.

> ::: dom/camera/GonkCameraControl.h
> @@ +44,5 @@
> > +
> > +  void ReceiveFrame(PRUint8 *aData, PRUint32 aLength);
> > +
> > +protected:
> > +  nsresult DoGetPreviewStream(GetPreviewStreamTask *aGetPreviewStream);
> 
> Ditto here. Sane naming please.

In this particular case, Do*() is how I've denoted the actual core implementation of the functionality, as opposed to async dispatch wrappers, etc.

> ::: dom/camera/GonkCameraControl.cpp
> @@ +56,5 @@
> > +  }
> > +}
> > +
> > +/*
> > +  Gonk-specific CameraControl implementation.
> 
> Can you use // for these kind of comments. /* */ on separate lines seems
> really wasteful. Sorry for the visual nits.

No problem--I'm a visual nit-ist myself.  Can we agree that cleaning up comment style is lower priority, if I promise to get it done next week?  :)

> @@ +171,5 @@
> > +        ++index;
> > +        /* intentional fallthrough */
> > +
> > +      case CAMERA_PARAM_FOCUSDISTANCEOPTIMUM:
> > +        ++index;
> 
> This is really magical. Calling a helper function is almost always preferred.

Normally I would agree, but in this case, breaking this out into a helper function just seems to make the code worse.  I can hammer on it if it's really important to you.

> @@ +302,5 @@
> > +          /* intentional fallthrough */
> > +
> > +        default:
> > +          DOM_CAMERA_LOGE("%s:%d : malformed region '%s'\n", __func__, __LINE__, p);
> > +          goto GetParameter_error;
> 
> can you do this with break? This is extremely ugly.

Agreed, but the error case is inside two for-loops.  Unfortunately it looks like even C++11 is missing named breaks, but I could do something like this: http://en.wikibooks.org/wiki/More_C%2B%2B_Idioms/Named_Loop

> @@ +339,5 @@
> > +    if (NS_IsMainThread()) {
> > +      nsCOMPtr<nsIRunnable> pushParametersTask = new PushParametersTask(this);
> > +      mCameraThread->Dispatch(pushParametersTask, NS_DISPATCH_NORMAL);
> > +    } else {
> > +      DoPushParameters(nsnull);
> 
> For consistency you could actually always dispatch (even when you are
> already on the camera thread). That would be less code.

In the case where I'm already on the camera thread, would Dispatch() invoke PushParametersTask::Run() right away, or defer until the current Run() is finished?  There are cases where code already running on the camera thread needs the parameter push to happen immediately/synchronously.

> @@ +440,5 @@
> > +{
> > +  nsCOMPtr<CameraPreview> preview = mPreview;
> > +
> > +  if (!preview) {
> > +    preview = new GonkCameraPreview(mHwHandle, aGetPreviewStream->mWidth, aGetPreviewStream->mHeight);
> 
> If !preview new preview
> 
> and then dispatch, no need to duplicate

Sorry, I'm not sure what you mean here.

> @@ +496,5 @@
> > +    mTakePictureOnSuccessCb = nsnull;
> > +    nsCOMPtr<nsICameraErrorCallback> ecb = mTakePictureOnErrorCb;
> > +    mTakePictureOnErrorCb = nsnull;
> > +    if (ecb) {
> > +      if (NS_FAILED(NS_DispatchToMainThread(new CameraErrorResult(ecb, NS_LITERAL_STRING("CANCELLED"))))) {
> 
> Don't call functions inside a macro.

Good catch!  In my own defence, I'm pretty sure I've seen this style used elsewhere in the gecko codebase. ;)  I'll fix all of these.

> ::: dom/camera/CameraNativeWindow.cpp
> @@ +185,5 @@
> > +                }
> > +            }
> > +        }
> > +
> > +#if 0 //XXX: Not sure if we need to do this check
> 
> ???
>
> @@ +284,5 @@
> > +    } else {
> > +        timestamp = mTimestamp;
> > +    }
> > +
> > +    //XXX:
> 
> ???

CameraNativeWindow.cpp is something I inherited, so I'm not sure what's going on here.

> @@ +303,5 @@
> > +int CameraNativeWindow::lockBuffer(ANativeWindowBuffer* buffer)
> > +{
> > +    CNW_LOGD("CameraNativeWindow::lockBuffer");
> > +    Mutex::Autolock lock(mMutex);
> > +    //TODO: Need to implement this.
> 
> ???
>
> @@ +318,5 @@
> > +        CNW_LOGE("cancelBuffer: slot index out of range [0, %d]: %d",
> > +                mBufferCount, buf);
> > +        return -EINVAL;
> > +    } else if (mSlots[buf].mBufferState != BufferSlot::DEQUEUED) {
> > +        printf_stderr("cancelBuffer: slot %d is not owned by the client (state=%d)",
>
> NS_WARNING or NS_ASSERTION?

Ditto.

> ::: dom/camera/deprecated_FallbackImpl.cpp
> @@ +50,5 @@
> > +    mTimer = nsnull;
> > +  }
> > +
> > +  void init() {
> > +    printf_stderr("XxXxX init()\n");
> 
> ???
> 
> @@ +65,5 @@
> > +  //
> > +  // MediaStreamListener
> > +  //
> > +  void NotifyBlockingChanged(MediaStreamGraph* aGraph, Blocking aBlocked) {
> > +    printf_stderr("XxXxX NotifyBlockingChanged blocked=%d\n", aBlocked);
> 
> ??? these printfs should go or become proper logging
> 
> @@ +145,5 @@
> > +
> > +    mNumFrames++;
> > +    mVideoSegment.AppendFrame(videoImage, 1, gfxIntSize(mWidth, mHeight));
> > +    mInput->AppendToTrack(TRACK_VIDEO, &mVideoSegment);
> > +    //PRUint32 time = SecondsToMediaTime(1 + mNumFrames / mFps);
> 
> ???

My apologies: the deprecated_*.cpp files really are deprecated; I removed a bunch of them, but missed this one.

> @@ +372,5 @@
> > +int
> > +GonkCameraHardware::doCameraHardwareTakePicture(PRUint32 aHwHandle)
> > +{
> > +  GonkCameraHardware* hw = getCameraHardware(aHwHandle);
> > +  if (hw) {
> 
> if !hw
>   return !OK; (is there ERROR or something like that?)

Looks like Android has a 'DEAD_OBJECT' return code, which I'll use.
(In reply to Boris Zbarsky (:bz) from comment #97)
> > I'll build one instance of the array object and return it on subsequent calls.
> 
> To be clear, I wasn't advocating for this.

I don't see a need to keep recreating the same array if it's not required.  I may defer this until after this week, though.

> That said, if you also freeze the array object, it might be ok.  Otherwise
> consumers will be able to modify it under you....

I was wondering about that.  Thanks.  Also, assuming the array is kept in a jsval member variable:
1. Is there a convenient notation for use in class initialize lists?
2. Is any DOM magic required to keep the jsval around, something akin to nsCOMPtr or similar?

Thanks!
(In reply to Johnny Stenback (:jst, jst@mozilla.com) from comment #90)
> Comment on attachment 642617 [details] [diff] [review]
> Part 3: new DOM framework
> 
> This generally looks good! I found a number of mostly minor things while
> reading through this, and I'm saying r- only because I want to look this
> over once more after the following stuff has been fixed. And we need to sort
> out the issue that bz pointed out as well.
> 
> Here's my findings:
> 
> - In setHelper() (and friends):
> 
> +  const char *v = ToNewCString(aValue);
> +  if (v) {
> +    aCameraContol->SetParameter(aKey, v);
> +    nsMemory::Free(const_cast<char*>(v));
> +    return NS_OK;
> +  } else {
> +    return NS_ERROR_OUT_OF_MEMORY;
> +  }
> 
> Remove the else after return, maybe simply return NS_ERROR_OUT_OF_MEMORY if
> (!v) here, and end with the actual logic in this function. This pattern
> appears in lots of these helpers, please change them all.
> 
> And in this particular version, any reason to not use
> NS_ConvertUTF16toUTF8(aValue).get()? That'll be faster for short strings,
> work with non-ascii characters, and be safe for long strings as well. And
> less code.

No reason at all except I didn't know about it.  Thanks!

> - In setHelper(nsCameraControl *aCameraContol, PRUint32 aKey, const
> JS::Value & aValue, JSContext *cx):
> 
> +      if (JS_GetArrayLength(cx, regions, &length)) {
> +        DOM_CAMERA_LOGI("%s:%d : got %d regions\n", __func__, __LINE__,
> length);
> +        parsedRegions = new nsCameraControl::CameraRegion[length];
> 
> Should we sanity check length here? What if I pass in a sparse array with a
> length of a billion or what not? We'll oom and die safely from the looks of
> things, but seems we can handle that better.
> 
> - In class GetPreviewStreamTask:
> 
> +  PRUint32 mWidth;
> +  PRUint32 mHeight;
> +  nsCOMPtr<nsCameraControl> mCameraControl;
> +  nsCOMPtr<nsICameraPreviewStreamCallback> mOnSuccessCb;
> +  nsCOMPtr<nsICameraErrorCallback> mOnErrorCb;
> 
> Probably want at least the last two of those to be protected.
>
> - In class AutoFocusTask:
> 
> +  nsCOMPtr<nsCameraControl> mCameraControl;
> +  nsCOMPtr<nsICameraAutoFocusCallback> mOnSuccessCb;
> +  nsCOMPtr<nsICameraErrorCallback> mOnErrorCb;
> 
> Same here and in TakePictureTask and StartRecordingTask as well.

They all originally were protected, but when I went on to subclass nsCameraControl into GonkCameraControl, that meant I would have to add the latter (and any other implementations) as friends of each of the *Task classes; I decided it was easier to just make all the members public (but I'm open to suggestions).

> - In nsDOMCameraManager::OnNavigation(PRUint64 aWindowId):
> 
> +  /* TODO: implement -- see getUserMedia() implementation */
> 
> This seems like something we want to do before we land this IMO.

I'm waiting for more information on what this event handler needs to do.

> - In nsDOMCameraManager::Create(PRUint64 aWindowId, nsDOMCameraManager *
> *aMozCameras):
> 
> +  /* TODO: check for permissions here to access cameras */
> 
> Same here.

Ditto.
> 1. Is there a convenient notation for use in class initialize lists?

JSVAL_VOID to set it to undefined.

> Is any DOM magic required to keep the jsval around

Lots, actually.  Especially if you want to avoid leaks.

Hence my questions about what you want the API to be long-term when this gets standardized, as opposed to what you're constrained to doing now by the fact that you're dealing with jsapi directly.
Attached patch WIP - new CameraControl API (obsolete) — Splinter Review
Monolithic patch that should include all suggested changes from the reviews as of Comment 101, except for the ones to which I explicitly responded.
Attachment #642667 - Attachment is obsolete: true
Attachment #642618 - Attachment is obsolete: true
Attachment #642618 - Flags: review?(roc)
Attachment #643223 - Flags: review?(roc)
Attached patch Part 3: new DOM framework (obsolete) — Splinter Review
Attachment #642617 - Attachment is obsolete: true
Attachment #643225 - Flags: review?(jst)
Attached patch Part 4: new Gonk-ICS support (obsolete) — Splinter Review
Attachment #642620 - Attachment is obsolete: true
Attachment #642620 - Flags: review?(gal)
Attachment #643227 - Flags: review?(gal)
Two problems observed on Otoro with this patch:
1. the recorded video is 0 bytes (log shows "W/GonkRecorder(  105): Failed to create the encoder")
2. switching back to still image mode causes the preview to freeze, the device then appears to run out of memory, and finally b2g crashes.
Attachment #642787 - Attachment is obsolete: true
Comment on attachment 643227 [details] [diff] [review]
Part 4: new Gonk-ICS support

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

Nice work overall. A lot of nits to fix. We are getting closer though.

::: dom/camera/FallbackCameraManager.cpp
@@ +3,5 @@
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "DOMCameraManager.h"
> +
> +

1 space here

@@ +4,5 @@
> +
> +#include "DOMCameraManager.h"
> +
> +
> +/*

/**                                                                                                                                                   
 * Create an <iframe mozbrowser> owned by the same document as                                                                                        
 * aOpenerFrameElement.                                                                                                                               
 */

This is the predominant comment style in dom/ for major multi-line comments on top of functions. In this case at most you need // From nsDOMCameraManager. since its a simple 1-line comment. Please change in the entire file(s).

::: dom/camera/GonkCameraHwMgr.h
@@ +16,5 @@
> +
> +#ifndef DOM_CAMERA_GONKCAMERAHWMGR_H
> +#define DOM_CAMERA_GONKCAMERAHWMGR_H
> +
> +

don't need two new lines here

@@ +45,5 @@
> +  GonkCameraHardware(GonkCamera* aTarget, PRUint32 aCamera);
> +  ~GonkCameraHardware();
> +  void init();
> +
> +  static void                   DataCallback(int32_t aMsgType, const sp<IMemory> &aDataPtr, camera_frame_metadata_t *aMetadata, void* aUser);

consistency of " *" and "* " please. Just search for those and make up your mind.

@@ +75,5 @@
> +protected:
> +  static GonkCameraHardware*    sHw;
> +  static PRUint32               sHwHandle;
> +
> +  static GonkCameraHardware*    getCameraHardware(PRUint32 aHwHandle)

Consistency. GetCameraHardware.

@@ +78,5 @@
> +
> +  static GonkCameraHardware*    getCameraHardware(PRUint32 aHwHandle)
> +  {
> +    if (aHwHandle == sHwHandle) {
> +      /*

Please never ever use this comment style:
/*
   Foo.
 */

Use // blocks or

/**
 *
 */

@@ +84,5 @@
> +        so even if this function is called with aHwHandle = 0, the
> +        result will still be null.
> +      */
> +      return sHw;
> +    } else {

Please look for more unnecessary else blocks.

@@ +112,5 @@
> +  struct timespec               mAutoFocusStart;
> +#endif
> +  bool                          mInitialized;
> +
> +  bool initialized()

IsInitialized()

::: dom/camera/GonkCameraControl.cpp
@@ +85,5 @@
> +  DOM_CAMERA_LOGI("%s:%d : this = %p, mHwHandle = %d\n", __func__, __LINE__, this, mHwHandle);
> +
> +  /* Initialize our camera configuration database. */
> +  mRwLock = PR_NewRWLock(PR_RWLOCK_RANK_NONE, "GonkCameraControl.Parameters.Lock");
> +  DoPullParameters(nsnull);

I thought we agreed on no more DoXXX.

@@ +130,5 @@
> +protected:
> +  PRRWLock *mRwLock;
> +};
> +
> +class RwAutoLockWrite

Please file a bug to move this code somewhere else (its of general use).

@@ +162,5 @@
> +  const char *key = getKeyText(aKey);
> +  if (key) {
> +    RwAutoLockRead lock(mRwLock);
> +    return mParams.get(key);
> +  } else {

dangling else, please search for all of these. And I recommend

if (!key)
  return nsnull;

@@ +195,5 @@
> +        s = mParams.get(key);
> +        if (!s) {
> +          return 0.0;
> +        }
> +        while (index) {

This kind of adhoc parsing of strings is really bad and a great way to get exploited. Why do we need this? Can you explain in the bug.

@@ +208,5 @@
> +
> +      default:
> +        return mParams.getFloat(key);
> +    }
> +  } else {

Another dangling else. You really like these. Flip this around as "if (!key) [newline] return (aKey == CAMERA_PARAM_ZOOM ? 1.0 : 0.0)". 2 lines instead of 10.

@@ +254,5 @@
> +  CameraRegion *regions = new CameraRegion[count];
> +  CameraRegion *r;
> +
> +  /* parse all of the region sets */
> +  char *end;

This is also high risk complex error prone parse code. Why? Can't we pass JS arrays of values around?

@@ +351,5 @@
> +nsGonkCameraControl::PushParameters()
> +{
> +  if (!mDeferConfigUpdate) {
> +    DOM_CAMERA_LOGI("%s:%d\n", __func__, __LINE__);
> +    /*  If we're already on the camera thread, call DoPushParameters()

Don't use this comment style. Why DoPushParameters() naming? And why not always dispatch?

@@ +399,5 @@
> +  }
> +
> +  {
> +    RwAutoLockWrite lock(mRwLock);
> +    switch (aKey) {

If you have one case, its an if, not a switch.

@@ +434,5 @@
> +  nsCString s;
> +
> +  for (PRUint32 i = 0; i < aLength; ++i) {
> +    CameraRegion *r = &aRegions[i];
> +    s.AppendPrintf("(%d,%d,%d,%d,%d),", r->mTop, r->mLeft, r->mBottom, r->mRight, r->mWeight);

Ok, much better. I assume the gonk abstraction wants this in string form?

@@ +460,5 @@
> +    preview = new GonkCameraPreview(mHwHandle, aGetPreviewStream->mWidth, aGetPreviewStream->mHeight);
> +    if (!preview) {
> +      rv = NS_DispatchToMainThread(new CameraErrorResult(aGetPreviewStream->mOnErrorCb, NS_LITERAL_STRING("OUT_OF_MEMORY")));
> +      if (NS_FAILED(rv)) {
> +        NS_WARNING("Failed to dispatch getPreviewStream() onError callback to main thread!");

just do NS_ENSURE_SUCCESS(rv, rv) in these cases (please check the whole file)

@@ +471,5 @@
> +  rv = NS_DispatchToMainThread(new GetPreviewStreamResult(preview.get(), aGetPreviewStream->mOnSuccessCb));
> +  if (NS_FAILED(rv)) {
> +    NS_WARNING("Failed to dispatch getPreviewStream() onSuccess callback to main thread!");
> +  }
> +  return NS_OK;

Aroo? You swallow the error condition here. Do return NS_DispatchToMainThread ...

@@ +475,5 @@
> +  return NS_OK;
> +}
> +
> +nsresult
> +nsGonkCameraControl::DoAutoFocus(AutoFocusTask *aAutoFocus)

Do?

@@ +485,5 @@
> +    nsCOMPtr<nsICameraErrorCallback> ecb = mAutoFocusOnErrorCb;
> +    mAutoFocusOnErrorCb = nsnull;
> +    if (ecb) {
> +      nsresult rv = NS_DispatchToMainThread(new CameraErrorResult(ecb, NS_LITERAL_STRING("CANCELLED")));
> +      if (NS_FAILED(rv)) {

NS_ENSURE_SUCCESS

@@ +498,5 @@
> +  mAutoFocusOnErrorCb = aAutoFocus->mOnErrorCb;
> +
> +  if (GonkCameraHardware::AutoFocus(mHwHandle) == OK) {
> +    return NS_OK;
> +  } else {

else ...

@@ +534,5 @@
> +  /* height and width: some drivers are less friendly about getting one of
> +     these set to zero, so if either is not specified, ignore both and go
> +     with current or default settings. */
> +  if (aTakePicture->mWidth && aTakePicture->mHeight) {
> +    if (snprintf(d, sizeof(d), "%dx%d", aTakePicture->mWidth, aTakePicture->mHeight) > 0) {

XPCOM AppendString stuff ...

@@ +548,5 @@
> +  if (mFileFormat) {
> +    nsMemory::Free(const_cast<char*>(mFileFormat));
> +  }
> +  mFileFormat = ToNewCString(aTakePicture->mFileFormat);
> +  if (mFileFormat) {

NS_ENSURE_TRUE(!!mFileFormat, NS_ERROR_OUT_OF_MEMORY)

@@ +562,5 @@
> +  r %= 360;
> +  r += 45;
> +  r /= 90;
> +  r *= 90;
> +  if (snprintf(d, sizeof(d), "%d", r) > 0) {

Dito. Below too. And use nice NS_ENSURE_SUCCESS macros with the new AppendString stuff.

@@ +608,5 @@
> +  mDeferConfigUpdate = false;
> +  PushParameters();
> +
> +  if (GonkCameraHardware::TakePicture(mHwHandle) == OK) {
> +    // mPreview->Stop();

? Please look for this kind of stuff so we don't have to iterate on it.

@@ +621,5 @@
> +{
> +  RwAutoLockRead lock(mRwLock);
> +  if (GonkCameraHardware::PushParameters(mHwHandle, mParams) == OK) {
> +    return NS_OK;
> +  } else {

else!

@@ +656,5 @@
> +    GonkCameraPreview *p = static_cast<GonkCameraPreview *>(preview.get());
> +    if (p) {
> +      p->ReceiveFrame(aData, aLength);
> +    } else {
> +      DOM_CAMERA_LOGE("%s:%d : weird pointer problem happened\n");

Scary. Want to explain or turn into an assert?

::: dom/camera/CameraNativeWindow.h
@@ +21,5 @@
> +{
> +public:
> +    enum { MIN_UNDEQUEUED_BUFFERS = 2 };
> +    enum {
> +        MIN_BUFFER_SLOTS  = MIN_UNDEQUEUED_BUFFERS

And this enum is formatted differently why?

::: dom/camera/CameraNativeWindow.cpp
@@ +185,5 @@
> +                }
> +            }
> +        }
> +
> +#if 0 //XXX: Not sure if we need to do this check

Please figure out what to do with this. I am flagging this for the 2nd time.

@@ +284,5 @@
> +    } else {
> +        timestamp = mTimestamp;
> +    }
> +
> +    //XXX:

???

@@ +303,5 @@
> +int CameraNativeWindow::lockBuffer(ANativeWindowBuffer* buffer)
> +{
> +    CNW_LOGD("CameraNativeWindow::lockBuffer");
> +    Mutex::Autolock lock(mMutex);
> +    //TODO: Need to implement this.

???

@@ +401,5 @@
> +{
> +    return NO_ERROR;
> +}
> +
> +//================================================

You don't use this //=== thing anywhere else

::: dom/camera/GonkCameraManager.cpp
@@ +72,5 @@
> +      /* TODO: clean up any partial objects? */
> +      return NS_ERROR_NOT_AVAILABLE;
> +    }
> +    jv = STRING_TO_JSVAL(v);
> +    JS_SetElement(cx, a, index++, &jv);

does this have a return value you are ignoring?

@@ +88,5 @@
> +  nsCOMPtr<nsICameraControl> cameraControl = new nsGonkCameraControl(mCameraId, mCameraThread);
> +
> +  DOM_CAMERA_LOGI("%s:%d\n", __func__, __LINE__);
> +
> +  nsresult rv = NS_DispatchToMainThread(new GetCameraResult(cameraControl, mOnSuccessCb));

commonly known, these 4 lines are also known as "return"

::: dom/camera/GonkCameraHwMgr.cpp
@@ +26,5 @@
> +
> +
> +USING_CAMERA_NAMESPACE
> +
> +#if GIHM_TIMING_RECEIVEFRAME

This is really not pretty.

@@ +49,5 @@
> +}
> +#endif
> +
> +
> +#if GIHM_STUB_NATIVEWINDOW

Do we ever use this?

@@ +195,5 @@
> +    mHardware.clear();
> +    return;
> +  }
> +
> +#if GIHM_STUB_NATIVEWINDOW

Do we ever use this?
(In reply to Mike Habicher [:mikeh] from comment #96)
> Right now, this is guaranteed by the values of lumaBpp (8) and chromaBpp (4).
> 
> This function is executed 30 times per second--I'd rather not add extra code
> if it's not strictly necessary.  Would some detailed comments suffice?

NS_ASSERTION compiles to nothing in release builds.
Also, doing a few instructions 30 times a second cannot have any observable effect on performance.
Comment on attachment 643223 [details] [diff] [review]
Part 2: new CameraPreview (MediaStream-specific code)

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

::: dom/camera/GonkCameraPreview.cpp
@@ +49,5 @@
> +        uint32_t uvN = yN / 4;
> +        uint32_t* src = (uint32_t*)( y + yN );
> +        uint32_t* d = new uint32_t[ uvN / 2 ];
> +        uint32_t* u = d;
> +        uint32_t* v = u + uvN / 4;

Please add an assertion that yN is divisible by 4 and uvN is divisible by 4.

@@ +67,5 @@
> +#define DEINTERLACE( u, v, s0, s1 )                             \
> +  u = ( (s0) & 0xFF00UL ) >> 8 | ( (s0) & 0xFF000000UL ) >> 16; \
> +  u |= ( (s1) & 0xFF00UL ) << 8 | ( (s1) & 0xFF000000UL );      \
> +  v = ( (s0) & 0xFFUL ) | ( (s0) & 0xFF0000UL ) >> 8;           \
> +  v |= ( (s1) & 0xFFUL ) << 16 | ( (s1) & 0xFF0000UL ) << 8;

this is still a bit opaque. A big comment and more meaningful variable names would help a lot. Just describing the layout you're converting from and to.

@@ +107,5 @@
> +  PlanarYCbCrImage::Data data;
> +  data.mYChannel = aData;
> +  data.mYSize = gfxIntSize(mWidth, mHeight);
> +  data.mYStride = mWidth * lumaBpp / 8.0;
> +  data.mCbCrStride = mWidth * chromaBpp / 8.0;

/8 ... no need for floating point arithmetic here.

@@ +133,5 @@
> +{
> +  DOM_CAMERA_LOGI("%s:%d : this=%p\n", __func__, __LINE__, this);
> +
> +  GonkCameraHardware::SetPreviewSize(mHwHandle, mWidth, mHeight);
> +  GonkCameraHardware::GetPreviewSize(mHwHandle, &mWidth, &mHeight);

This looks odd. Add a comment explaining why you need to set and then get?

::: dom/camera/CameraPreview.h
@@ +18,5 @@
> +using namespace mozilla::layers;
> +
> +BEGIN_CAMERA_NAMESPACE
> +
> +static const TrackID TRACK_AUDIO = 1;

Remove this since you don't need it
(In reply to Mike Habicher [:mikeh] from comment #93)
> > ::: dom/camera/GonkCameraHwMgr.cpp
> > @@ +38,5 @@
> > > +}
> > > +#endif
> > > +
> > > +
> > > +#if GIHM_STUB_NATIVEWINDOW
> > 
> > Is there ever a case where we'd prefer the stub window over the native
> > window?
> 
> Sometimes, particular if it's giving us trouble on a new platform.  e.g. the
> Nexus S initially didn't work with native windows enabled, and early porting
> was done with the stub windows.

Hm I think it's better to just remove this then. That bit working better with the Nexus S sounds like a one off thing.
(In reply to Andreas Gal :gal from comment #107)
> Comment on attachment 643227 [details] [diff] [review]
> Part 4: new Gonk-ICS support
> 
> Review of attachment 643227 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nice work overall. A lot of nits to fix. We are getting closer though.

--- 8< ---

> @@ +75,5 @@
> > +protected:
> > +  static GonkCameraHardware*    sHw;
> > +  static PRUint32               sHwHandle;
> > +
> > +  static GonkCameraHardware*    getCameraHardware(PRUint32 aHwHandle)
> 
> Consistency. GetCameraHardware.

I'm going to drop this to GetHardware().

> @@ +78,5 @@
> > +
> > +  static GonkCameraHardware*    getCameraHardware(PRUint32 aHwHandle)
> > +  {
> > +    if (aHwHandle == sHwHandle) {
> > +      /*
> 
> Please never ever use this comment style:
> /*
>    Foo.
>  */
> 
> Use // blocks or
> 
> /**
>  *
>  */

Okay, but...

<rant>Both of these formats are _horrible_ to maintain in multiline comments.  Any changes that need the comment text reflowed mean going through and manually fixing up any *'s or //'s that wind up in the middle of lines.  Only /* ... */ doesn't require this.</rant>

> ::: dom/camera/GonkCameraControl.cpp
> @@ +85,5 @@
> > +  DOM_CAMERA_LOGI("%s:%d : this = %p, mHwHandle = %d\n", __func__, __LINE__, this, mHwHandle);
> > +
> > +  /* Initialize our camera configuration database. */
> > +  mRwLock = PR_NewRWLock(PR_RWLOCK_RANK_NONE, "GonkCameraControl.Parameters.Lock");
> > +  DoPullParameters(nsnull);
> 
> I thought we agreed on no more DoXXX.

I've removed the (admittedly silly) do*() functions from GonkCameraHardware API, but I don't think we came to any agreement on GonkCameraControl.  I'd like to keep this particular set of functions named this way to make it obvious they're the core camera-thread implementations, instead of some overloaded version of the DOM APIs (which are already called GetPreviewStream, TakePicture, etc.).  If it's the Do*() that really bugs you, how about *Impl(), e.g. TakePictureImpl()?

> @@ +130,5 @@
> > +protected:
> > +  PRRWLock *mRwLock;
> > +};
> > +
> > +class RwAutoLockWrite
> 
> Please file a bug to move this code somewhere else (its of general use).

Done: bug 775106.

> @@ +195,5 @@
> > +        s = mParams.get(key);
> > +        if (!s) {
> > +          return 0.0;
> > +        }
> > +        while (index) {
> 
> This kind of adhoc parsing of strings is really bad and a great way to get
> exploited. Why do we need this? Can you explain in the bug.

Will replace with sscanf() (unless there's a better Moz-specific function, but I couldn't find anything obvious in MDN).

> @@ +208,5 @@
> > +
> > +      default:
> > +        return mParams.getFloat(key);
> > +    }
> > +  } else {
> 
> Another dangling else. You really like these.

More that I don't dislike them... but your point is made. :)

> @@ +254,5 @@
> > +  CameraRegion *regions = new CameraRegion[count];
> > +  CameraRegion *r;
> > +
> > +  /* parse all of the region sets */
> > +  char *end;
> 
> This is also high risk complex error prone parse code. Why? Can't we pass JS
> arrays of values around?

Yes, but as you noted (below), the gonk layer only deals with strings containing a list of 5-tuples, so the complex parsing will be required at some point.

I'll rework this to make it simpler.

> @@ +351,5 @@
> > +nsGonkCameraControl::PushParameters()
> > +{
> > +  if (!mDeferConfigUpdate) {
> > +    DOM_CAMERA_LOGI("%s:%d\n", __func__, __LINE__);
> > +    /*  If we're already on the camera thread, call DoPushParameters()
> 
> Don't use this comment style. Why DoPushParameters() naming? And why not
> always dispatch?

If PushParameters() is called from the camera thread, it needs to be handled synchronously, so instead of the Dispatch(), it just calls the implementation directly.  I'll document that in the function.

> @@ +399,5 @@
> > +  }
> > +
> > +  {
> > +    RwAutoLockWrite lock(mRwLock);
> > +    switch (aKey) {
> 
> If you have one case, its an if, not a switch.

I was future-proofing. :)

> @@ +434,5 @@
> > +  nsCString s;
> > +
> > +  for (PRUint32 i = 0; i < aLength; ++i) {
> > +    CameraRegion *r = &aRegions[i];
> > +    s.AppendPrintf("(%d,%d,%d,%d,%d),", r->mTop, r->mLeft, r->mBottom, r->mRight, r->mWeight);
> 
> Ok, much better. I assume the gonk abstraction wants this in string form?

Correct.

> @@ +562,5 @@
> > +  r %= 360;
> > +  r += 45;
> > +  r /= 90;
> > +  r *= 90;
> > +  if (snprintf(d, sizeof(d), "%d", r) > 0) {
> 
> Dito. Below too. And use nice NS_ENSURE_SUCCESS macros with the new
> AppendString stuff.

I'm not sure what you mean: AppendPrintf() returns void, and I couldn't find any reference in mxr/MDN to AppendString().

> @@ +88,5 @@
> > +  nsCOMPtr<nsICameraControl> cameraControl = new nsGonkCameraControl(mCameraId, mCameraThread);
> > +
> > +  DOM_CAMERA_LOGI("%s:%d\n", __func__, __LINE__);
> > +
> > +  nsresult rv = NS_DispatchToMainThread(new GetCameraResult(cameraControl, mOnSuccessCb));
> 
> commonly known, these 4 lines are also known as "return"

Oy. Thanks.
> <rant>Both of these formats are _horrible_ to maintain in multiline
> comments.  Any changes that need the comment text reflowed mean going
> through and manually fixing up any *'s or //'s that wind up in the middle of
> lines.  Only /* ... */ doesn't require this.</rant>

Its about consistency. I don't really like any of these comment styles and have my own favorite ...
> you, how about *Impl(), e.g. TakePictureImpl()?

Sounds good.

> Yes, but as you noted (below), the gonk layer only deals with strings
> containing a list of 5-tuples, so the complex parsing will be required at
> some point.
> 
> I'll rework this to make it simpler.

Meh. Ok. This kinda blows. sscanf is probably your best bet here.

> I'm not sure what you mean: AppendPrintf() returns void, and I couldn't find
> any reference in mxr/MDN to AppendString().

Ok, use AppendPrintf, even better. Its infallible.

We should be good after this iteration. Do you need me to take over any of the other reviews or are those moving along?
(In reply to Michael Wu [:mwu] from comment #86)
> Comment on attachment 642620 [details] [diff] [review]
> Part 4: new Gonk-ICS support
> 
> Review of attachment 642620 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> CameraNativeWindow.cpp needs a license.

Okay, turns out CameraNativeWindow.* are derived heavily from SurfaceTexture.* in Android, so I've prepended the Apache 2 license with (C) AOSP.  Should they also get a (C) Mozilla Foundation statement?

(These will also be renamed GonkNativeWindow.* to reflect their platform-specificity.)
(In reply to Andreas Gal :gal from comment #113)
> We should be good after this iteration. Do you need me to take over any of
> the other reviews or are those moving along?

See comment 114 about the license/copyright notice in CameraNativeWindow.*.  Some advice on that would be appreciated.
(In reply to Andreas Gal :gal from comment #113)
> Ok, use AppendPrintf, even better. Its infallible.

Quick question: I couldn't find a method to clear the nsCString, aside from calling Truncate() with no parameter (which causes it to default to 0).  Any sense on what's a better approach:

nsCString s;
s.AppendPrintf(...);
SetParameter(..., s.get());
s.Truncate();
s.AppendPrintf(...);
SetParameter(..., s.get());
etc.

...or...

{
  nsCString s;
  s.AppendPrintf(...);
  SetParameter(..., s.get());
}
{
  nsCString s;
  s.AppendPrintf(...);
  SetParameter(..., s.get());
}

...?

Thank!
How about

  SetParameter(..., nsPrintfCString("%d", val).get());
(In reply to Justin Lebar [:jlebar] from comment #117)
> How about
> 
>   SetParameter(..., nsPrintfCString("%d", val).get());

That works!  Thanks.
Attached patch WIP - new CameraControl API (obsolete) — Splinter Review
Latest patch, which should address everyone's issues.  Broken-out patches coming soon.
Attachment #643221 - Attachment is obsolete: true
Attached patch Part 4: new Gonk-ICS support (obsolete) — Splinter Review
Attachment #643227 - Attachment is obsolete: true
Attachment #643227 - Flags: review?(gal)
Attachment #643538 - Flags: review?(gal)
Attached patch Part 3: new DOM framework (obsolete) — Splinter Review
Attachment #643225 - Attachment is obsolete: true
Attachment #643225 - Flags: review?(jst)
Attachment #643539 - Flags: review?(jst)
Attachment #643223 - Attachment is obsolete: true
Attachment #643223 - Flags: review?(roc)
Attachment #643540 - Flags: review?(roc)
Comment on attachment 643540 [details] [diff] [review]
Part 2: new CameraPreview (MediaStream-specific code)

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

::: dom/camera/GonkCameraPreview.cpp
@@ +83,5 @@
> +        // we're handling pairs of 32-bit words, so divide by 8
> +        NS_ASSERTION(uvN & 0x7 == 0, "Invalid image dimensions!");
> +        uvN /= 8;
> +
> +        while( uvN-- ) {

while (uvN--)
Attachment #643540 - Flags: review?(roc) → review+
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #123)
> Comment on attachment 643540 [details] [diff] [review]
> Part 2: new CameraPreview (MediaStream-specific code)
> 
> Review of attachment 643540 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/camera/GonkCameraPreview.cpp
> @@ +83,5 @@
> > +        // we're handling pairs of 32-bit words, so divide by 8
> > +        NS_ASSERTION(uvN & 0x7 == 0, "Invalid image dimensions!");
> > +        uvN /= 8;
> > +
> > +        while( uvN-- ) {
> 
> while (uvN--)

Thanks.  I'll fix this.

jst/gal, review ping?
Attached patch Camera Facing Fix (obsolete) — Splinter Review
MikeH, You need to apply this Michael's patch to fix the camera facing. With this patch the facing and orientation should work on all devices: Otoro, Akami and Maguro.
Comment on attachment 644017 [details] [diff] [review]
Camera Facing Fix

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

How does this handle the case where there is more than one camera, but one of them doesn't have |info.facing| set to either CAMERA_FACING_FRONT or CAMERA_FACING_BACK?
do we care?  this isn't the case for otoro or akami.  we can support the full universe of cameras in a follow-on patch.
Attachment #643538 - Flags: review?(gal) → review+
Comment on attachment 643539 [details] [diff] [review]
Part 3: new DOM framework

This is getting close! Couple of minor things below...

+//  Helpers for reading optional integer properties.
+static PRUint32
+getPropertyHelper(JSContext *cx, JSObject *o, const char *prop, PRUint32 aDefault)
+{
+  jsval p;
+  if (JS_GetProperty(cx, o, prop, &p) && JSVAL_IS_INT(p)) {
+    return JSVAL_TO_INT(p);
+  }
+  return aDefault;
+}
+
+static PRInt32
+getPropertyHelper(JSContext *cx, JSObject *o, const char *prop, PRInt32 aDefault)
+{
+  jsval p;
+  if (JS_GetProperty(cx, o, prop, &p) && JSVAL_IS_INT(p)) {
+    return JSVAL_TO_INT(p);
+  }
+  return aDefault;
+}

Rather than two *identical* functions, why not just have one and add casts in the couple of places where you'd need to? Especially since nothing here does any bounds checking here... Which begs the question, should we check? I.e. if the unsigned version gets called, but the value is actually negative... should we return the default value, or just cast (implicitly or explicitly) and let the value go through and end up being a huge value unsigned value?

+static nsresult
+getHelper(nsCameraControl *aCameraControl, PRUint32 aKey, nsAString& aValue)
+{
+  const char *value = aCameraControl->GetParameterConstChar(aKey);
+  if (value) {
+    return NS_ERROR_FAILURE;

I presume you meant if (!value) there...

- In setHelper(nsCameraControl *aCameraContol, PRUint32 aKey, const JS::Value & aValue, JSContext *cx, PRUint32 aLimit):

+    if (JS_IsArrayObject(cx, regions)) {
+      if (JS_GetArrayLength(cx, regions, &length)) {

Merge the two if's?

- In nsCameraControl::GetCapabilities():

+    capabilities = new nsCameraCapabilities(this);
+    if (!capabilities) {
+      return NS_ERROR_OUT_OF_MEMORY;
+    }

No need to check, new is infallible.

With that fixed, the TODO's dealt with, and bz's array issue sorted out I think this is good to go. r- only becase I want to read over the array stuff, and freezing of the array etc. You'll need to root the array there one way or another, so the JS GC doesn't collect it and leave you with a dangling pointer. #jsapi on irc is your friend for getting the latest on how to do that, but probably more importantly, we need to unroot the array at some point too, or we'll leak (the array will hold its parent alive, which is likely a window object, which will hold lots and lots of stuff alive).
Attachment #643539 - Flags: review?(jst) → review-
(In reply to Johnny Stenback (:jst, jst@mozilla.com) from comment #128)
> Comment on attachment 643539 [details] [diff] [review]
> Part 3: new DOM framework
> 
> This is getting close! Couple of minor things below...
> 
> +//  Helpers for reading optional integer properties.
> +static PRUint32
> +getPropertyHelper(JSContext *cx, JSObject *o, const char *prop, PRUint32
> aDefault)
> +{
> +  jsval p;
> +  if (JS_GetProperty(cx, o, prop, &p) && JSVAL_IS_INT(p)) {
> +    return JSVAL_TO_INT(p);
> +  }
> +  return aDefault;
> +}
> +
> +static PRInt32
> +getPropertyHelper(JSContext *cx, JSObject *o, const char *prop, PRInt32
> aDefault)
> +{
> +  jsval p;
> +  if (JS_GetProperty(cx, o, prop, &p) && JSVAL_IS_INT(p)) {
> +    return JSVAL_TO_INT(p);
> +  }
> +  return aDefault;
> +}
> 
> Rather than two *identical* functions, why not just have one and add casts
> in the couple of places where you'd need to? Especially since nothing here
> does any bounds checking here... Which begs the question, should we check?
> I.e. if the unsigned version gets called, but the value is actually
> negative... should we return the default value, or just cast (implicitly or
> explicitly) and let the value go through and end up being a huge value
> unsigned value?

I was thinking about this last night.  How about instead of just JSVAL_TO_INT(),
JS_ValueToECMAInt/Uint32() ?

> +static nsresult
> +getHelper(nsCameraControl *aCameraControl, PRUint32 aKey, nsAString& aValue)
> +{
> +  const char *value = aCameraControl->GetParameterConstChar(aKey);
> +  if (value) {
> +    return NS_ERROR_FAILURE;
> 
> I presume you meant if (!value) there...

How embarassing.

> With that fixed, the TODO's dealt with, and bz's array issue sorted out I
> think this is good to go. r- only becase I want to read over the array
> stuff, and freezing of the array etc. You'll need to root the array there
> one way or another, so the JS GC doesn't collect it and leave you with a
> dangling pointer. #jsapi on irc is your friend for getting the latest on how
> to do that, but probably more importantly, we need to unroot the array at
> some point too, or we'll leak (the array will hold its parent alive, which
> is likely a window object, which will hold lots and lots of stuff alive).

Right now I'm returning new arrays on each capability attribute query.  Is
that okay for now, and something I can address with a frozen singleton in a
future change?

Regarding the TODOs [grep...]:

CameraControl.cpp:113:          // TODO: move the Gonk-specific default values somewhere else
CameraControl.cpp:552:   * TODO: pick up the actual specified picture format for the MIME type;
DOMCameraManager.cpp:47:  // TODO: implement -- see getUserMedia() implementation
DOMCameraManager.cpp:54:  // TODO: check for permissions here to access cameras
DOMCameraManager.h:70:    // TODO: window management stuff
GonkCameraManager.cpp:70:        // TODO: handle extra cameras in getCamera().

I'm still waiting to hear back on [DOMCameraManager.cpp:47], [DOMCameraManager.cpp:54] and [DOMCameraManager.h:70], and the others are nits that aren't broken for the Gonk implementation--they just need to be cleaned up.

Would it be okay with you if I filed follow-up bugs to address these?  I'm hoping to get this change landed soon so that people can start using it in b2g.

Thanks!
Attached patch WIP - new CameraControl API (obsolete) — Splinter Review
Latest monolithic patch that should include all feedback from everyone.  (A broken up series of patches is pending.)

Updated video recording WIP patch will be added to bug 776062.
Attachment #643250 - Attachment is obsolete: true
Attachment #643534 - Attachment is obsolete: true
Fixes the whitespace bug observed by :roc.
Attachment #643540 - Attachment is obsolete: true
Attached patch Part 3: new DOM framework (obsolete) — Splinter Review
Addresses most recent issues identified by :jst.

TODOs as identified in my most recent reply remain.
Attachment #643539 - Attachment is obsolete: true
Attached patch Part 4: new Gonk-ICS support (obsolete) — Splinter Review
Includes camera facing fix.
Attachment #643538 - Attachment is obsolete: true
Attachment #644017 - Attachment is obsolete: true
Attachment #644483 - Flags: review?(gal)
Attachment #644482 - Flags: review?(jst)
Comment on attachment 644483 [details] [diff] [review]
Part 4: new Gonk-ICS support

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

::: dom/camera/GonkNativeWindow.cpp
@@ +179,5 @@
> +    bool tryAgain = true;
> +
> +    CNW_LOGD("dequeueBuffer: E");
> +    while (tryAgain) {
> +

No space after {

@@ +324,5 @@
> +}
> +
> +int GonkNativeWindow::perform(int operation, va_list args)
> +{
> +    int res = NO_ERROR;

remove res

@@ +329,5 @@
> +
> +    switch (operation) {
> +    case NATIVE_WINDOW_CONNECT:
> +        // deprecated. must return NO_ERROR.
> +        break;

return NO_ERROR

@@ +334,5 @@
> +    case NATIVE_WINDOW_DISCONNECT:
> +        // deprecated. must return NO_ERROR.
> +        break;
> +    case NATIVE_WINDOW_SET_USAGE:
> +        res = dispatchSetUsage(args);

return instead of res =
Attachment #644483 - Flags: review?(gal) → review+
review ping, :jst ?
Comment on attachment 644482 [details] [diff] [review]
Part 3: new DOM framework

+//  Helpers for reading optional integer properties.
+static PRUint32
+getPropertyHelper(JSContext *cx, JSObject *o, const char *prop, PRUint32 aDefault)
+{
+  PRUint32 u;
+  jsval v;
+
+  if (JS_GetProperty(cx, o, prop, &v) && JS_ValueToECMAUint32(cx, v, &u)) {
+    return u;
+  }
+  return aDefault;
+}
+
+static PRInt32
+getPropertyHelper(JSContext *cx, JSObject *o, const char *prop, PRInt32 aDefault)
+{
+  PRInt32 i;
+  jsval v;
+
+  if (JS_GetProperty(cx, o, prop, &v) && JS_ValueToECMAInt32(cx, v, &i)) {
+    return i;
+  }
+  return aDefault;
+}

I still don't get the need for two helpers here, please just use the signed version and cast at the callers (that's the *only* difference between JS_ValueToECMAUint32() and JS_ValueToECMAInt32()).

The rest looks good, but the last paragraph of my previous review feedback (comment 128) still applies, and r- until that's dealt with.
Attachment #644482 - Flags: review?(jst) → review-
The "options" objects should be using dictionaries instead of reinventing that wheel.

The array case should be using a sequence when we move to webidl, though we'll need to add support for sequences of dictionaries; in the meantime I guess it's sort of fine (apart from being totally unsafe with moving gc).
Comment on attachment 644482 [details] [diff] [review]
Part 3: new DOM framework

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

Uh, no, this needs review by someone who knows JSAPI. In the first place, if a function that returns JSBool (like JS_GetProperty) returns false, you need to immediately return an exception.
Er, and that too.  ;)  

Fwiw, jst and I do both know jsapi....
And that brings me to...

Apart from sequences of dictionaries, what else blocks this from using webidl?  If it's just sequences of dictionaries, it might be worth just fixing that instead of trying to fix all the jsapi stuff here.
Though smaug points out you can use xpidl dictionaries by hand from C++ code, so you could do those in setHelper as well, and not need to do any of that getPropertyHelper stuff at all.
Oh, one last thing.  the JS_IsArrayObject thing in setHelper is presumably unnecessary: JS_GetArrayLength and JS_GetElement are both generic.
Attachment #642566 - Flags: review?(jst) → review+
(In reply to Boris Zbarsky (:bz) from comment #140)
> And that brings me to...
> 
> Apart from sequences of dictionaries, what else blocks this from using
> webidl?  If it's just sequences of dictionaries, it might be worth just
> fixing that instead of trying to fix all the jsapi stuff here.

I think time pressure is the one thing preventing us from doing that here. That is IMO the next step here though.
Attached patch WIP - new CameraControl API (obsolete) — Splinter Review
With feedback from #jsapi, this monolithic patch uses xpidl dictionaries to handle incoming parameters.  It also ensures that every JS_*() function call that returns false immediately returns an error code so that any enqueued exceptions can be handled.

To get this to build requires a patch from bug 777074.
Attachment #644476 - Attachment is obsolete: true
Attached patch Part 1: new CameraControl IDL (obsolete) — Splinter Review
Adds dictionaries to simplify parsing of inbound objects.
Attachment #642566 - Attachment is obsolete: true
Attachment #642566 - Flags: review?(jonas)
Attachment #645777 - Flags: review?(jst)
Attached patch Part 3: new DOM framework (obsolete) — Splinter Review
Replace the incoming JS object parsing code with greatly-simplified dictionaries.  Outgoing objects still require use of JS_*() functions.  Make sure than whenever a JS_*() function fails (i.e. returns false) we immediately bail and return an NS_ERROR code.

Also call JS_FreezeObject() on returned capabilities arrays.
Attachment #644482 - Attachment is obsolete: true
Attachment #645780 - Flags: review?(jst)
Attachment #645780 - Flags: review?(bzbarsky)
Attachment #645780 - Flags: review?(Ms2ger)
Attached patch Part 4: new Gonk-ICS support (obsolete) — Splinter Review
Address :gal's last feedback.
Attachment #644483 - Attachment is obsolete: true
Comment on attachment 645780 [details] [diff] [review]
Part 3: new DOM framework

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

First pass:

::: dom/camera/CameraControl.cpp
@@ +29,5 @@
> +NS_IMPL_RELEASE(nsCameraControl)
> +
> +// Helpers for string properties.
> +static inline nsresult
> +setHelper(nsCameraControl *aCameraContol, PRUint32 aKey, const nsAString& aValue)

* to the left everywhere

@@ +32,5 @@
> +static inline nsresult
> +setHelper(nsCameraControl *aCameraContol, PRUint32 aKey, const nsAString& aValue)
> +{
> +  aCameraContol->SetParameter(aKey, NS_ConvertUTF16toUTF8(aValue).get());
> +  return NS_OK;

Not sure if we need two-line helpers like this

@@ +36,5 @@
> +  return NS_OK;
> +}
> +
> +static nsresult
> +getHelper(nsCameraControl *aCameraControl, PRUint32 aKey, nsAString& aValue)

These should just be private member functions, if anything.

@@ +65,5 @@
> +}
> +
> +// Helper for weighted regions.
> +static nsresult
> +setHelper(nsCameraControl *aCameraContol, PRUint32 aKey, const JS::Value & aValue, JSContext *cx, PRUint32 aLimit)

cx should be the first argument

@@ +72,5 @@
> +    DOM_CAMERA_LOGI("%s:%d : aLimit = 0, nothing to do\n", __func__, __LINE__);
> +    return NS_OK;
> +  }
> +
> +  PRUint32 length = 0;

Declare lower, and use uint32_t.

@@ +74,5 @@
> +  }
> +
> +  PRUint32 length = 0;
> +
> +  if (aValue.isObject()) {

if (!aValue.isObject()) {
  return NS_ERROR_something;
}

@@ +75,5 @@
> +
> +  PRUint32 length = 0;
> +
> +  if (aValue.isObject()) {
> +    JSObject *regions = JSVAL_TO_OBJECT(aValue);

&aValue.toObject()

@@ +83,5 @@
> +
> +    DOM_CAMERA_LOGI("%s:%d : got %d regions (limited to %d)\n", __func__, __LINE__, length, aLimit);
> +    if (length > aLimit) {
> +      length = aLimit;
> +    }

Not sure what's this about?

@@ +89,5 @@
> +    nsTArray<CameraRegion> regionArray;
> +    regionArray.SetCapacity(length);
> +
> +    for (PRUint32 i = 0; i < length; ++i) {
> +      jsval v;

JS::Value

@@ +143,5 @@
> +  for (PRUint32 i = 0; i < length; ++i) {
> +    CameraRegion *r = &regionArray[i];
> +    JS::Value v;
> +
> +    JSObject *o = JS_NewObject(cx, nsnull, nsnull, nsnull);

This should just use an interface, in an nsIVariant array

@@ +195,5 @@
> +    capabilities = new nsCameraCapabilities(this);
> +    mCapabilities = capabilities;
> +  }
> +
> +  capabilities.forget(aCapabilities);

if (!mCapabilities) {
  mCapabilities) = new nsCameraCapabilities(this);
}

nsCOMPtr<nsICameraCapabilities> capabilities = mCapabilities;
capabilities.forget(aCapabilities);

@@ +213,5 @@
> +}
> +
> +/* attribute DOMString whiteBalanceMode; */
> +NS_IMETHODIMP
> +nsCameraControl::GetWhiteBalanceMode(nsAString & aWhiteBalanceMode)

& to the left, btw

@@ +339,5 @@
> +  if (JSVAL_IS_NULL(aCompensation) || JSVAL_IS_VOID(aCompensation)) {
> +    /* use NaN to switch the camera back into auto mode */
> +    return setHelper(this, CAMERA_PARAM_EXPOSURECOMPENSATION, NAN);
> +  }
> +  return NS_ERROR_INVALID_ARG;

Just use JS_ValueToNumber.

@@ +417,5 @@
> +
> +  return NS_OK;
> +}
> +
> +/* void takePicture (in nsICameraTakePictureCallback onSuccess, [optional] in nsICameraErrorCallback onError); */

Looks wrong. Just remove those lines.

@@ +433,5 @@
> +  nsresult rv = aOptions->GetPictureSize(&pictureSize);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  rv = size.Init(cx, &pictureSize);
> +  NS_ENSURE_SUCCESS(rv, rv);

Dict please

@@ +452,5 @@
> +   * NaN indicates no value provided.
> +   */
> +  pos.latitude = NAN;
> +  pos.longitude = NAN;
> +  pos.altitude = NAN;

Where does this NAN symbol come from?

@@ +471,5 @@
> +   * Auto focusing can change some of the camera's parameters, so
> +   * we need to pull a new set before sending the result to the
> +   * main thread.
> +   */
> +  PullParametersImpl(nsnull);

nullptr

@@ +492,5 @@
> +  /**
> +   * TODO: pick up the actual specified picture format for the MIME type;
> +   * for now, assume we'll be using JPEGs.
> +   */
> +  nsIDOMBlob *blob = new nsDOMMemoryFile((void*)data, (PRUint64)aLength, NS_LITERAL_STRING("image/jpeg"));

C++ casts, please

::: dom/camera/CameraPictureOptions.cpp
@@ +1,4 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +

hg rm

::: dom/camera/DOMCameraManager.h
@@ +18,5 @@
> +public:
> +  NS_DECL_ISUPPORTS
> +  NS_DECL_NSIDOMCAMERAMANAGER
> +
> +  static NS_IMETHODIMP Create(PRUint64 aWindowId, nsDOMCameraManager * *aMozCameras);

"static NS_IMETHODIMP" doesn't make sense. In fact, this should just return already_AddRefed<nsDOMCameraManager>.

@@ +27,5 @@
> +  nsDOMCameraManager();
> +  nsDOMCameraManager(PRUint64 aWindowId);
> +  nsDOMCameraManager(const nsDOMCameraManager&);
> +  nsDOMCameraManager& operator=(const nsDOMCameraManager&);
> +  ~nsDOMCameraManager();

Some MOZ_DELETEs?

::: dom/camera/CameraCommon.h
@@ +14,5 @@
> +  do {                                  \
> +    if ( DOM_CAMERA_LOG_LEVEL >= (l) ) {  \
> +      printf_stderr (__VA_ARGS__); \
> +    }                                   \
> +  } while (0)

Do something with those backslashes, please

@@ +41,5 @@
> +  NS_IMETHOD Run()
> +  {
> +    MOZ_ASSERT(NS_IsMainThread());
> +
> +    if (mOnErrorCb) {

Not sure what the point is in dispatching this if the callback is null

::: dom/camera/CameraControl.h
@@ +67,5 @@
> +  virtual const char* GetParameterConstChar(PRUint32 aKey) = 0;
> +  virtual double GetParameterDouble(PRUint32 aKey) = 0;
> +  virtual void GetParameter(PRUint32 aKey, nsTArray<CameraRegion>& aRegions) = 0;
> +  virtual void SetParameter(const char *aKey, const char *aValue) = 0;
> +  virtual void SetParameter(PRUint32 aKey, const char *aValue) = 0;

Just pass smart strings through; I think you're probably going to stick pointers to dead memory somewhere

@@ +92,5 @@
> +
> +  virtual ~nsCameraControl()
> +  {
> +    if (mFileFormat) {
> +      nsMemory::Free(const_cast<char*>(mFileFormat));

Ugh.

::: dom/camera/CameraCapabilities.cpp
@@ +28,5 @@
> +NS_IMPL_RELEASE(nsCameraCapabilities)
> +
> +
> +nsCameraCapabilities::nsCameraCapabilities(nsCameraControl *aCamera) :
> +  mCamera(aCamera)

: on this line

@@ +66,5 @@
> +static nsresult
> +parseStringItemAndAdd(JSContext *cx, JSObject *array, PRUint32 index, const char *start, char **end)
> +{
> +  JSString* v;
> +  jsval jv;

'v' tends to be used for jsvals; can the string be 'string' or 's'?

@@ +77,5 @@
> +
> +  if (!v) {
> +    return NS_ERROR_OUT_OF_MEMORY;
> +  }
> +  jv = STRING_TO_JSVAL(v);

Declare jv here

@@ +91,5 @@
> +{
> +  char* x;
> +  jsval w;
> +  jsval h;
> +  jsval v;

Declare later

@@ +122,5 @@
> +  return NS_OK;
> +}
> +
> +nsresult
> +nsCameraCapabilities::parameterListToNewArray(JSContext *cx, JSObject **array, const char *key, nsresult (*parseItemAndAdd)(JSContext *cx, JSObject *array, PRUint32 index, const char *start,  char **end))

Use a typedef for the function pointer.

@@ +153,5 @@
> +      index += 1;
> +    }
> +    p = q;
> +    if (p) {
> +      p += 1;

++p

@@ +161,5 @@
> +  if (JS_FreezeObject(cx, *array)) {
> +    return NS_OK;
> +  } else {
> +    return NS_ERROR_FAILURE;
> +  }

I'm not convinces this is necessary, but if you think it is, use ?: please

@@ +168,5 @@
> +nsresult
> +nsCameraCapabilities::stringListToNewObject(JSContext* cx, JS::Value *aArray, const char *key)
> +{
> +  JSObject* array;
> +  nsresult rv;

Declare rv on first use

@@ +178,5 @@
> +  return NS_OK;
> +}
> +
> +nsresult
> +nsCameraCapabilities::dimensionListToNewObject(JSContext* cx, JS::Value *aArray, const char *key)

Oh, and all these functions need to start with a capital letter

@@ +327,5 @@
> +    *aMaxMeteringAreas = 0;
> +    return NS_OK;
> +  }
> +
> +  *aMaxMeteringAreas = atoi(value);

Ugh, atoi

@@ +339,5 @@
> +  NS_ENSURE_TRUE(mCamera, NS_ERROR_NOT_AVAILABLE);
> +
> +  const char* value;
> +  JSObject* array;
> +  nsresult rv;

Declare on first use

@@ +342,5 @@
> +  JSObject* array;
> +  nsresult rv;
> +
> +  value = mCamera->GetParameter(CameraParameters::KEY_ZOOM_SUPPORTED);
> +  if (!value || strcmp(value, CameraParameters::TRUE) != 0) {

Ugh, strcmp

::: dom/base/Navigator.cpp
@@ +1344,5 @@
> +  if (!cameraManager) {
> +    nsCOMPtr<nsPIDOMWindow> win = do_QueryReferent(mWindow);
> +    NS_ENSURE_TRUE(win, NS_ERROR_FAILURE);
> +
> +    if (!win || !win->GetOuterWindow() ||

Handling null win twice here

@@ +1355,5 @@
> +
> +    cameraManager = mCameraManager;
> +  }
> +
> +  cameraManager.forget(aCameraManager);

Move the local here, as in the other place I commented on this pattern

@@ +1393,5 @@
> +#endif
> +#ifdef MOZ_B2G_CAMERA
> +  if (mCameraManager) {
> +    mCameraManager->OnNavigation(win->WindowID());
> +  }

Pre-existing bug: I don't see anything that guarantees that win is not null.

::: dom/camera/CameraPictureOptions.h
@@ +11,5 @@
> +#include "CameraCommon.h"
> +
> +namespace mozilla {
> +
> +class nsCameraPictureOptions : public nsICameraPictureOptions

This class can go entirely

::: dom/camera/Makefile.in
@@ +5,5 @@
> +DEPTH		         = ../..
> +topsrcdir	       = @top_srcdir@
> +srcdir		       = @srcdir@
> +VPATH		         = @srcdir@
> +

Weird mix of spaces and tabs. Just use one space.
Comment on attachment 645780 [details] [diff] [review]
Part 3: new DOM framework

(Oh, and I need to touch that flag, I guess.)
Attachment #645780 - Flags: review?(Ms2ger)
Comment on attachment 645780 [details] [diff] [review]
Part 3: new DOM framework

jst will handle the review...
Attachment #645780 - Flags: review?(bzbarsky)
(In reply to :Ms2ger from comment #148)
> Comment on attachment 645780 [details] [diff] [review]
> Part 3: new DOM framework
> 
> Review of attachment 645780 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> First pass:
> 
> ::: dom/camera/CameraControl.cpp
> @@ +29,5 @@
> > +NS_IMPL_RELEASE(nsCameraControl)
> > +
> > +// Helpers for string properties.
> > +static inline nsresult
> > +setHelper(nsCameraControl *aCameraContol, PRUint32 aKey, const nsAString& aValue)
> 
> * to the left everywhere

Can we _please_ leave this to a follow-up change?

> @@ +32,5 @@
> > +static inline nsresult
> > +setHelper(nsCameraControl *aCameraContol, PRUint32 aKey, const nsAString& aValue)
> > +{
> > +  aCameraContol->SetParameter(aKey, NS_ConvertUTF16toUTF8(aValue).get());
> > +  return NS_OK;
> 
> Not sure if we need two-line helpers like this

I wanted to keep everything as consistent as possible.  This way, if the SetHelper()s need to change in the future, it's just in one place.

> @@ +72,5 @@
> > +    DOM_CAMERA_LOGI("%s:%d : aLimit = 0, nothing to do\n", __func__, __LINE__);
> > +    return NS_OK;
> > +  }
> > +
> > +  PRUint32 length = 0;
> 
> Declare lower, and use uint32_t.

Just here or everywhere?  (It looks like both are used in dom/*, but PRUint32 is by far the more common.)

> @@ +83,5 @@
> > +
> > +    DOM_CAMERA_LOGI("%s:%d : got %d regions (limited to %d)\n", __func__, __LINE__, length, aLimit);
> > +    if (length > aLimit) {
> > +      length = aLimit;
> > +    }
> 
> Not sure what's this about?

Back in comment 90, :jst raised a concern about someone passing us a very large sparse array.  In the interests of sanity checking the array length, we compare it against the maximum number of elements that particular option can handle, and limit it to that.

> @@ +143,5 @@
> > +  for (PRUint32 i = 0; i < length; ++i) {
> > +    CameraRegion *r = &regionArray[i];
> > +    JS::Value v;
> > +
> > +    JSObject *o = JS_NewObject(cx, nsnull, nsnull, nsnull);
> 
> This should just use an interface, in an nsIVariant array

I'm sorry, I don't understand what you mean.

> @@ +213,5 @@
> > +}
> > +
> > +/* attribute DOMString whiteBalanceMode; */
> > +NS_IMETHODIMP
> > +nsCameraControl::GetWhiteBalanceMode(nsAString & aWhiteBalanceMode)
> 
> & to the left, btw

Will fix these up with the *'s later.

> @@ +339,5 @@
> > +  if (JSVAL_IS_NULL(aCompensation) || JSVAL_IS_VOID(aCompensation)) {
> > +    /* use NaN to switch the camera back into auto mode */
> > +    return setHelper(this, CAMERA_PARAM_EXPOSURECOMPENSATION, NAN);
> > +  }
> > +  return NS_ERROR_INVALID_ARG;
> 
> Just use JS_ValueToNumber.

According to MDN, JS_ValueToNumber() returns 0 for null, but 0 is a valid exposure compensation value, so it won't work here.

> @@ +452,5 @@
> > +   * NaN indicates no value provided.
> > +   */
> > +  pos.latitude = NAN;
> > +  pos.longitude = NAN;
> > +  pos.altitude = NAN;
> 
> Where does this NAN symbol come from?

#include <math.h>

> @@ +471,5 @@
> > +   * Auto focusing can change some of the camera's parameters, so
> > +   * we need to pull a new set before sending the result to the
> > +   * main thread.
> > +   */
> > +  PullParametersImpl(nsnull);
> 
> nullptr

This would make this the only occurrence of 'nullptr' in dom/*.  Why the change?

> ::: dom/camera/CameraControl.h
> @@ +67,5 @@
> > +  virtual const char* GetParameterConstChar(PRUint32 aKey) = 0;
> > +  virtual double GetParameterDouble(PRUint32 aKey) = 0;
> > +  virtual void GetParameter(PRUint32 aKey, nsTArray<CameraRegion>& aRegions) = 0;
> > +  virtual void SetParameter(const char *aKey, const char *aValue) = 0;
> > +  virtual void SetParameter(PRUint32 aKey, const char *aValue) = 0;
> 
> Just pass smart strings through; I think you're probably going to stick
> pointers to dead memory somewhere

The 'const char *aKey' pointers all refer to strings that exist in a Gonk/Android class.  If nsCameraControl exists, they exist.

I started looking into doing this, and it's a HUGE change.  If you have a specific concern that needs addressing, let me know; else can we move this to another bug and tackle it later on?

> @@ +327,5 @@
> > +    *aMaxMeteringAreas = 0;
> > +    return NS_OK;
> > +  }
> > +
> > +  *aMaxMeteringAreas = atoi(value);
> 
> Ugh, atoi

:) Is there a better way to do this?
Attached patch WIP - new CameraControl API (obsolete) — Splinter Review
Monolithic patch incorporating feedback from :ms2ger.

Apparently NS_OUTPARAM has been removed from m-c; this patch is based on m-b2g/m-c, which still has those symbols.  If you run into a build error, just removed those symbols.
Attachment #645774 - Attachment is obsolete: true
Attached patch Part 1: new CameraControl IDL (obsolete) — Splinter Review
Replaces interface nsICameraPictureOptions with dictionary CameraPictureOptions.
Attachment #645777 - Attachment is obsolete: true
Attachment #645777 - Flags: review?(jst)
Attachment #645978 - Flags: review?(jst)
Attached patch Part 3: new DOM framework (obsolete) — Splinter Review
Incorporate feedback from :ms2ger
- add MOZ_DELETE to private copy constructors and operator= methods
- change DOMCameraManager::Create() to return already_AddRefed<nsDOMCameraManager>
- restructure singleton creators in CameraCapabilities and Navigator::GetMozCameras()
- change TakePicture() to use dictionary CameraPictureOptions
- made SetHelper()s and GetHelper()s members of nsCameraControl
- error Runnable is no longer dispatched if onError wasn't specified
- mFileFormat is now a smart string
- general clean-up
Attachment #645780 - Attachment is obsolete: true
Attachment #645780 - Flags: review?(jst)
Attachment #645979 - Flags: review?(jst)
> This would make this the only occurrence of 'nullptr' in dom/*.  Why the change?

We just landed the ability to use nullptr.  I wouldn't worry about it too much, since there will be a mass-change from nsnull to nullptr in the near future.

As far as NaNs, if you're handing them out to JS please use js_NaN, because for JS not all NaNs are created equal.  If you're just using them internally it probably doesn't matter what you use.
(In reply to Mike Habicher [:mikeh] from comment #151)
> (In reply to :Ms2ger from comment #148)
> > Comment on attachment 645780 [details] [diff] [review]
> > Part 3: new DOM framework
> > 
> > Review of attachment 645780 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > First pass:
> > 
> > ::: dom/camera/CameraControl.cpp
> > @@ +29,5 @@
> > > +NS_IMPL_RELEASE(nsCameraControl)
> > > +
> > > +// Helpers for string properties.
> > > +static inline nsresult
> > > +setHelper(nsCameraControl *aCameraContol, PRUint32 aKey, const nsAString& aValue)
> > 
> > * to the left everywhere
> 
> Can we _please_ leave this to a follow-up change?

Why? That would unnecessarily obscure blame.

> > @@ +72,5 @@
> > > +    DOM_CAMERA_LOGI("%s:%d : aLimit = 0, nothing to do\n", __func__, __LINE__);
> > > +    return NS_OK;
> > > +  }
> > > +
> > > +  PRUint32 length = 0;
> > 
> > Declare lower, and use uint32_t.
> 
> Just here or everywhere?  (It looks like both are used in dom/*, but
> PRUint32 is by far the more common.)

Just when you're passing pointers to integers into JSAPI; those functions take stdint types, and they are not necessarily compatible with the PR types.

> > @@ +83,5 @@
> > > +
> > > +    DOM_CAMERA_LOGI("%s:%d : got %d regions (limited to %d)\n", __func__, __LINE__, length, aLimit);
> > > +    if (length > aLimit) {
> > > +      length = aLimit;
> > > +    }
> > 
> > Not sure what's this about?
> 
> Back in comment 90, :jst raised a concern about someone passing us a very
> large sparse array.  In the interests of sanity checking the array length,
> we compare it against the maximum number of elements that particular option
> can handle, and limit it to that.

I think it makes more sense to use fallible allocation, and just throw if we fail to allocate.

> > @@ +143,5 @@
> > > +  for (PRUint32 i = 0; i < length; ++i) {
> > > +    CameraRegion *r = &regionArray[i];
> > > +    JS::Value v;
> > > +
> > > +    JSObject *o = JS_NewObject(cx, nsnull, nsnull, nsnull);
> > 
> > This should just use an interface, in an nsIVariant array
> 
> I'm sorry, I don't understand what you mean.

Something like <http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/HTMLPropertiesCollection.cpp#467>.

> > @@ +339,5 @@
> > > +  if (JSVAL_IS_NULL(aCompensation) || JSVAL_IS_VOID(aCompensation)) {
> > > +    /* use NaN to switch the camera back into auto mode */
> > > +    return setHelper(this, CAMERA_PARAM_EXPOSURECOMPENSATION, NAN);
> > > +  }
> > > +  return NS_ERROR_INVALID_ARG;
> > 
> > Just use JS_ValueToNumber.
> 
> According to MDN, JS_ValueToNumber() returns 0 for null, but 0 is a valid
> exposure compensation value, so it won't work here.

Why does null need to be handled differently here? If you need that...

if (aCompensation.isNullOrUndefined()) {
  // Do something weird.
}

// Call JS_ValueToNumber().
Attached patch WIP - new CameraControl API (obsolete) — Splinter Review
Monolithic patch rebased on m/m-c; removes NS_OUTPARAM.  No functional change intended.
Attachment #645975 - Attachment is obsolete: true
(In reply to :Ms2ger from comment #156)
> (In reply to Mike Habicher [:mikeh] from comment #151)
> > (In reply to :Ms2ger from comment #148)
> > > Comment on attachment 645780 [details] [diff] [review]
> > > Part 3: new DOM framework
> > > 
> > > Review of attachment 645780 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > First pass:
> > >
> > > @@ +83,5 @@
> > > > +
> > > > +    DOM_CAMERA_LOGI("%s:%d : got %d regions (limited to %d)\n", __func__, __LINE__, length, aLimit);
> > > > +    if (length > aLimit) {
> > > > +      length = aLimit;
> > > > +    }
> > > 
> > > Not sure what's this about?
> > 
> > Back in comment 90, :jst raised a concern about someone passing us a very
> > large sparse array.  In the interests of sanity checking the array length,
> > we compare it against the maximum number of elements that particular option
> > can handle, and limit it to that.
> 
> I think it makes more sense to use fallible allocation, and just throw if we
> fail to allocate.

I think it's simpler to limit the list to a reasonable number (which the underlying driver is happy to provide us) and go with that.  The camera driver will ignore any extras anyway--no need to waste memory on them.

> > > @@ +143,5 @@
> > > > +  for (PRUint32 i = 0; i < length; ++i) {
> > > > +    CameraRegion *r = &regionArray[i];
> > > > +    JS::Value v;
> > > > +
> > > > +    JSObject *o = JS_NewObject(cx, nsnull, nsnull, nsnull);
> > > 
> > > This should just use an interface, in an nsIVariant array
> > 
> > I'm sorry, I don't understand what you mean.
> 
> Something like
> <http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/
> HTMLPropertiesCollection.cpp#467>.

As discussed in IRC, will leave this as-is.
Attached patch WIP - new CameraControl API (obsolete) — Splinter Review
Monolithic patch, based on feedback from :ms2ger.
Attachment #646187 - Attachment is obsolete: true
Cosmetic fixes, no functional changes intended.
Attachment #644480 - Attachment is obsolete: true
Attached patch Part 3: new DOM framework (obsolete) — Splinter Review
Based on feedback from :ms2ger.
Attachment #645979 - Attachment is obsolete: true
Attachment #645979 - Flags: review?(jst)
Attachment #646271 - Flags: review?(jst)
Attached patch Part 4: new Gonk-ICS support (obsolete) — Splinter Review
Cosmetic changes, no functional change intended.
Attachment #645782 - Attachment is obsolete: true
Attached patch WIP - new CameraControl API (obsolete) — Splinter Review
Cosmetic fixen.  No functional change intended.
Attachment #646269 - Attachment is obsolete: true
Attached patch Part 4: new Gonk-ICS support (obsolete) — Splinter Review
Cosmetic fixen.  No functional change intended.
Attachment #646272 - Attachment is obsolete: true
Attached patch WIP - new CameraControl API (obsolete) — Splinter Review
Cosmetic fixen.  My 'Foo *foo' to 'Foo* foo' script missed types that end in numerics, e.g. 'PRUint32'.  No functional change intended.
Attachment #646305 - Attachment is obsolete: true
Attached patch Part 4: new Gonk-ICS support (obsolete) — Splinter Review
Cosmetic fixen.  My 'Foo *foo' to 'Foo* foo' script missed types that end in numerics, e.g. 'PRUint32'.  No functional change intended.
Attachment #646307 - Attachment is obsolete: true
Attached patch WIP - new CameraControl API (obsolete) — Splinter Review
Fix some incorrect symbols in dictionary_helper_gen.conf, even though their being wrong didn't seem to affect the build at all.
Attachment #646324 - Attachment is obsolete: true
Attached patch Part 3: new DOM framework (obsolete) — Splinter Review
Fix some incorrect symbol names in dictionary_helper_gen.conf, even though their being wrong didn't seem to affect the build at all.  No functional change intended.
Attachment #646271 - Attachment is obsolete: true
Attachment #646271 - Flags: review?(jst)
Attachment #646349 - Flags: review?(jst)
Comment on attachment 646349 [details] [diff] [review]
Part 3: new DOM framework

Looks good! r=jst, but please change NAN to js_NaN per bz's suggestion.

And the TODO's still need to be dealt with, but maybe do that in followup patches? (or submit interdiffs from now on)
Attachment #646349 - Flags: review?(jst) → review+
Attachment #645978 - Flags: review?(jst) → review+
(In reply to Johnny Stenback (:jst, jst@mozilla.com) from comment #169)
> Comment on attachment 646349 [details] [diff] [review]
> Part 3: new DOM framework
> 
> Looks good! r=jst, but please change NAN to js_NaN per bz's suggestion.
> 
> And the TODO's still need to be dealt with, but maybe do that in followup
> patches? (or submit interdiffs from now on)

The TODOs will be dealt with in follow-up patches.

Where can I find js_NaN?  I tried jsnum.h, but I get build errors: "error: 'js_NaN' was not declared in this scope"

std::numeric_limits<double>::quiet_NaN() seems to work, though.
Hrm.  Didn't realize that was hidden.

For code passing the NaN down to C++, it doesn't matter what NaN you use.

For code returning a NaN into JS, if you're not manually putting it into a jsval (which I seemed to see in some earlier patches in this bug), it doesn't matter which NaN you use.  So in particular, XPConnect methods that have a double* out param can use any NaN you want; XPConnect makes sure to use JS apis that canonicalize the NaN.
Attached patch WIP - new CameraControl API (obsolete) — Splinter Review
Monolithic patch for ICS camera support for B2G.  Includes the last of the feedback plus non-functional changes required to not break non-B2G builds.

B2G build tested locally; non-B2G builds stub out the mozCameras attribute on Navigator so that it only returns NS_ERROR_NOT_IMPLEMENTED.

Try: https://tbpl.mozilla.org/?tree=Try&rev=8785047e735d
Attachment #645978 - Attachment is obsolete: true
Attachment #646270 - Attachment is obsolete: true
Attachment #646325 - Attachment is obsolete: true
Attachment #646346 - Attachment is obsolete: true
Attachment #646349 - Attachment is obsolete: true
Attached patch new CameraControl API (obsolete) — Splinter Review
Two fixen:
1. #define NAN std::numeric_limits<double>::quiet_NaN() for Visual Studio
2. remove MOZ_B2G_CAMERA=1 from configure.in to enable fallback camera support in all builds; ICS-based builds will pick up this setting from mozconfig.

Try: https://tbpl.mozilla.org/?tree=Try&rev=e8802a4033d6
Attachment #647274 - Attachment is obsolete: true
Attachment #647296 - Attachment is obsolete: true
I grabbed the patch from try and did s/nsnull/nullptr/g.

https://hg.mozilla.org/integration/mozilla-inbound/rev/d59f932deea9
Apparently the patch on try is different from the patch posted here, and the patch on try has the wrong version of GonkCameraCapabilities.cpp . I pushed a follow up to switch to the right version.

https://hg.mozilla.org/integration/mozilla-inbound/rev/b13039139d7b
Blocks: 779351
>+@BINPATH@/components/dom_camera.xpt

Was added to b2g, browser, mobile/xul package-maniftest.in, but was not added to mobile/android. Was this on purpose or just missed?
Please replace http://hg.mozilla.org/mozilla-central/annotate/f24229bc0ec8/dom/camera/CameraCommon.h#l25 with something else.
We don't want stuff to be printed to stderr all the time, even on desktop builds.
...I see the following occasionally when running opt FF build
/home/smaug/mozilla/hg/mozilla/dom/camera/DOMCameraManager.cpp:36
/home/smaug/mozilla/hg/mozilla/dom/camera/DOMCameraManager.cpp:36
/home/smaug/mozilla/hg/mozilla/dom/camera/DOMCameraManager.cpp:42
/home/smaug/mozilla/hg/mozilla/dom/camera/DOMCameraManager.cpp:36
/home/smaug/mozilla/hg/mozilla/dom/camera/DOMCameraManager.cpp:42
/home/smaug/mozilla/hg/mozilla/dom/camera/DOMCameraManager.cpp:42
(In reply to Mark Finkle (:mfinkle) from comment #179)
> >+@BINPATH@/components/dom_camera.xpt
> 
> Was added to b2g, browser, mobile/xul package-maniftest.in, but was not
> added to mobile/android. Was this on purpose or just missed?

Just missed.  Sorry.  Feel free to add it, or I can (later).
No longer blocks: 790746
Depends on: 790746
(In reply to Mike Habicher [:mikeh] from comment #182)
> (In reply to Mark Finkle (:mfinkle) from comment #179)
> > >+@BINPATH@/components/dom_camera.xpt
> > 
> > Was added to b2g, browser, mobile/xul package-maniftest.in, but was not
> > added to mobile/android. Was this on purpose or just missed?
> 
> Just missed.  Sorry.  Feel free to add it, or I can (later).

It's not added yet to mobile/android, right?
Blocks: 803471
No longer blocks: 803471
You need to log in before you can comment on or make changes to this bug.