[:fabrice] Fabrice Desré Reporter
	Description • 13 years ago

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).

	Mike Habicher [:mikeh] (high bugzilla latency) Assignee
	Comment 1 • 13 years ago

I can take a look at this.  What hardware is this happening on?

	[:fabrice] Fabrice Desré Reporter
	Comment 2 • 13 years ago

(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.

	Mike Habicher [:mikeh] (high bugzilla latency) Assignee
	Comment 3 • 13 years ago

(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.

	Mike Habicher [:mikeh] (high bugzilla latency) Assignee
	Comment 4 • 13 years ago

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;
}

	Mike Habicher [:mikeh] (high bugzilla latency) Assignee
	Comment 5 • 13 years ago

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.

	Mike Habicher [:mikeh] (high bugzilla latency) Assignee
	Comment 6 • 13 years ago

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

	Mike Habicher [:mikeh] (high bugzilla latency) Assignee
	Comment 7 • 13 years ago

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

	Inder
	Comment 8 • 13 years ago

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?

	[:fabrice] Fabrice Desré Reporter
	Comment 9 • 13 years ago

(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.

	Mike Habicher [:mikeh] (high bugzilla latency) Assignee
	Comment 11 • 13 years ago

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.

	Inder
	Comment 12 • 13 years ago

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

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
	Comment 13 • 13 years ago

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?

	Mike Habicher [:mikeh] (high bugzilla latency) Assignee
	Comment 14 • 13 years ago

(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.

	[:fabrice] Fabrice Desré Reporter
	Comment 15 • 13 years ago

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

	Inder
	Comment 16 • 13 years ago

(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.

	[:fabrice] Fabrice Desré Reporter
	Comment 17 • 13 years ago

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.

	[:fabrice] Fabrice Desré Reporter
	Comment 18 • 13 years ago

Sorry, I was testing with the wrong build :(
Still crashing without --disable-jemalloc, but I've not gave up yet

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
	Comment 19 • 12 years ago

If there's no better way, you can call __real_malloc or whatever the name is that --wrap generates.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
	Comment 20 • 12 years ago

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.)

	Michael Wu [:mwu]
	Comment 21 • 12 years ago

(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.

	Justin Lebar (not reading bugmail)
	Comment 22 • 12 years ago

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?

	Mike Hommey [:glandium]
	Comment 23 • 12 years ago

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.

	Andreas Gal :gal
	Comment 24 • 12 years ago

I think we should fix this. We will use more mobile HAL APIs than mobile fennec (camera, gralloc, etc).

	Mike Hommey [:glandium]
	Comment 25 • 12 years ago

It would seem it's already the default...
http://android.git.linaro.org/gitweb?p=platform/bionic.git;a=blob;f=linker/linker.c;h=bcfa8dc768c9261e03b58c6d2cec28b5219a0d28;hb=6baffed252d0efbce5a79f356c2f156f4f818996#l54

It's weird, because i thought it didn't work...

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
	Comment 26 • 12 years ago

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).

	Michael Wu [:mwu]
	Comment 27 • 12 years ago

(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

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
	Comment 28 • 12 years ago

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.

	Michael Wu [:mwu]
	Comment 29 • 12 years ago

(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.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
	Comment 30 • 12 years ago

OK.  Then LD_PRELOAD for gonk and turning off --wrap sounds like a good solution to me.

	Mike Habicher [:mikeh] (high bugzilla latency) Assignee
	Comment 31 • 12 years ago

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.

	Mike Habicher [:mikeh] (high bugzilla latency) Assignee
	Comment 32 • 12 years ago

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.

	Mike Habicher [:mikeh] (high bugzilla latency) Assignee
	Comment 33 • 12 years ago

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.)

	Mike Habicher [:mikeh] (high bugzilla latency) Assignee
	Comment 34 • 12 years ago

Set viewfinder.src = '' in beforeunload instead of unload, to make sure we hit the right window.

	Mike Habicher [:mikeh] (high bugzilla latency) Assignee
	Comment 35 • 12 years ago

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.

	Mike Habicher [:mikeh] (high bugzilla latency) Assignee
	Comment 36 • 12 years ago

Also includes a hack to restart the preview.  See notes with camera.js.

	Mike Habicher [:mikeh] (high bugzilla latency) Assignee
	Comment 37 • 12 years ago

As before, but replace .src='' with .src=null, which eliminates GeckoConsole errors.

	Mike Habicher [:mikeh] (high bugzilla latency) Assignee
	Comment 38 • 12 years ago

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.)

	Mike Habicher [:mikeh] (high bugzilla latency) Assignee
	Comment 39 • 12 years ago

Functionally equivalent to attachment 625165 [details] [diff] [review], but with GonkCamera and GonkCameraHardware broken out into their own files, easier to read.

	Mike Habicher [:mikeh] (high bugzilla latency) Assignee
	Comment 40 • 12 years ago

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.

	Mike Habicher [:mikeh] (high bugzilla latency) Assignee
	Comment 41 • 12 years ago

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.

	:gerard-majax [PTO 01/08/2023-10/09/2023]
	Comment 43 • 12 years ago

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 ?

	:gerard-majax [PTO 01/08/2023-10/09/2023]
	Comment 44 • 12 years ago

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

	[:fabrice] Fabrice Desré Reporter
	Comment 45 • 12 years ago

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.

	Lawrence Mandel [:lmandel] (use needinfo)
	Comment 46 • 12 years ago

Blocking on a decision at the very least.

	Mike Habicher [:mikeh] (high bugzilla latency) Assignee
	Comment 47 • 12 years ago

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?

	Lawrence Mandel [:lmandel] (use needinfo)
	Comment 48 • 12 years ago

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?

	:gerard-majax [PTO 01/08/2023-10/09/2023]
	Comment 49 • 12 years ago

Done, I filed bug 761521 for this.

	Mike Habicher [:mikeh] (high bugzilla latency) Assignee
	Comment 50 • 12 years ago

(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.

	Lawrence Mandel [:lmandel] (use needinfo)
	Comment 51 • 12 years ago

(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?

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
	Comment 53 • 12 years ago

Comment on attachment 631543 [details] [diff] [review]
Rebased on latest b2g gecko rev

git screwed this up, ignore.

	JP Rosevear [:jpr]
	Comment 56 • 12 years ago

We upgraded lower level pieces and broke camera support, needs to be fixed.

	Mike Habicher [:mikeh] (high bugzilla latency) Assignee
	Comment 57 • 12 years ago

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.

	Mike Habicher [:mikeh] (high bugzilla latency) Assignee
	Comment 58 • 12 years ago

For use with the new CameraControl API - as noted on that attachment, numerous lifecycle issues remain.

	Johnny Stenback (:jst)
	Comment 63 • 12 years ago

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?).

	Mike Habicher [:mikeh] (high bugzilla latency) Assignee
	Comment 64 • 12 years ago

(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!

	Johnny Stenback (:jst)
	Comment 65 • 12 years ago

(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.

	Mike Habicher [:mikeh] (high bugzilla latency) Assignee
	Comment 66 • 12 years ago

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.

	Mike Habicher [:mikeh] (high bugzilla latency) Assignee
	Comment 67 • 12 years ago

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.

	Mike Habicher [:mikeh] (high bugzilla latency) Assignee
	Comment 68 • 12 years ago

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.

	Mike Habicher [:mikeh] (high bugzilla latency) Assignee
	Comment 69 • 12 years ago

Incremental patch that includes files missing from last night's diff.  Apologies.

	Mike Habicher [:mikeh] (high bugzilla latency) Assignee
	Comment 70 • 12 years ago

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

	Mike Habicher [:mikeh] (high bugzilla latency) Assignee
	Comment 71 • 12 years ago

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().

	Mike Habicher [:mikeh] (high bugzilla latency) Assignee
	Comment 72 • 12 years ago

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)

	Mike Habicher [:mikeh] (high bugzilla latency) Assignee
	Comment 73 • 12 years ago

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).

	Mike Habicher [:mikeh] (high bugzilla latency) Assignee
	Comment 74 • 12 years ago

Comment on attachment 637879 [details] [diff] [review]
Part 1: new CameraControl IDL

Tagging as a patch.

	Michael Wu [:mwu]
	Comment 75 • 12 years ago

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.

	Mike Habicher [:mikeh] (high bugzilla latency) Assignee
	Comment 76 • 12 years ago

Includes a fix for the single-camera Otoro, and adds better handling of invalid cameras.

	Mike Habicher [:mikeh] (high bugzilla latency) Assignee
	Comment 77 • 12 years ago

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.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
	Comment 78 • 12 years ago

Mike, what's blocking here other than review from sicking and jst?  How's the implementation looking?

sicking/jst, review ping :).

	Mike Habicher [:mikeh] (high bugzilla latency) Assignee
	Comment 79 • 12 years ago

(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.

	Mike Habicher [:mikeh] (high bugzilla latency) Assignee
	Comment 80 • 12 years ago

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.)

	Mike Habicher [:mikeh] (high bugzilla latency) Assignee
	Comment 81 • 12 years ago

Latest version of the DOM code and build framework (makefiles, etc.)

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 85 • 12 years ago

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.

	Michael Wu [:mwu]
	Comment 86 • 12 years ago

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

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 87 • 12 years ago

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?

	Inder
	Comment 88 • 12 years ago

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

	Boris Zbarsky [:bzbarsky]
	Comment 89 • 12 years ago

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).

	Johnny Stenback (:jst)
	Comment 90 • 12 years ago

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.

	Mike Habicher [:mikeh] (high bugzilla latency) Assignee
	Comment 91 • 12 years ago

This builds but is not yet tested.  Parking it here for now because I haven't had the best luck with laptop hard drives....

	Andreas Gal :gal
	Comment 92 • 12 years ago

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?)

	Mike Habicher [:mikeh] (high bugzilla latency) Assignee
	Comment 93 • 12 years ago

(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.

	Mike Habicher [:mikeh] (high bugzilla latency) Assignee
	Comment 94 • 12 years ago

(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.

	Mike Habicher [:mikeh] (high bugzilla latency) Assignee
	Comment 95 • 12 years ago

(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.

	Mike Habicher [:mikeh] (high bugzilla latency) Assignee
	Comment 96 • 12 years ago

(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?

	Boris Zbarsky [:bzbarsky]
	Comment 97 • 12 years ago

> 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....

	Mike Habicher [:mikeh] (high bugzilla latency) Assignee
	Comment 98 • 12 years ago

(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.

	Mike Habicher [:mikeh] (high bugzilla latency) Assignee
	Comment 99 • 12 years ago

(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!

	Mike Habicher [:mikeh] (high bugzilla latency) Assignee
	Comment 100 • 12 years ago

(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.

	Boris Zbarsky [:bzbarsky]
	Comment 101 • 12 years ago

> 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.

	Mike Habicher [:mikeh] (high bugzilla latency) Assignee
	Comment 102 • 12 years ago

Monolithic patch that should include all suggested changes from the reviews as of Comment 101, except for the ones to which I explicitly responded.

	Mike Habicher [:mikeh] (high bugzilla latency) Assignee
	Comment 106 • 12 years ago

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.

	Andreas Gal :gal
	Comment 107 • 12 years ago

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?

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 108 • 12 years ago

(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.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 109 • 12 years ago

Also, doing a few instructions 30 times a second cannot have any observable effect on performance.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 110 • 12 years ago

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

	Michael Wu [:mwu]
	Comment 111 • 12 years ago

(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.

	Mike Habicher [:mikeh] (high bugzilla latency) Assignee
	Comment 112 • 12 years ago

(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.

	Andreas Gal :gal
	Comment 113 • 12 years ago

> <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?

	Mike Habicher [:mikeh] (high bugzilla latency) Assignee
	Comment 114 • 12 years ago

(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.)

	Mike Habicher [:mikeh] (high bugzilla latency) Assignee
	Comment 115 • 12 years ago

(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.

	Mike Habicher [:mikeh] (high bugzilla latency) Assignee
	Comment 116 • 12 years ago

(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!

	Justin Lebar (not reading bugmail)
	Comment 117 • 12 years ago

How about

  SetParameter(..., nsPrintfCString("%d", val).get());

	Mike Habicher [:mikeh] (high bugzilla latency) Assignee
	Comment 118 • 12 years ago

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

That works!  Thanks.

	Mike Habicher [:mikeh] (high bugzilla latency) Assignee
	Comment 119 • 12 years ago

Latest patch, which should address everyone's issues.  Broken-out patches coming soon.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 123 • 12 years ago

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--)

	Mike Habicher [:mikeh] (high bugzilla latency) Assignee
	Comment 124 • 12 years ago

(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?

	Inder
	Comment 125 • 12 years ago

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.

	Mike Habicher [:mikeh] (high bugzilla latency) Assignee
	Comment 126 • 12 years ago

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?

	Michael Vines [:m1] [:evilmachines]
	Comment 127 • 12 years ago

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.

	Johnny Stenback (:jst)
	Comment 128 • 12 years ago

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).

	Mike Habicher [:mikeh] (high bugzilla latency) Assignee
	Comment 129 • 12 years ago

(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!

	Mike Habicher [:mikeh] (high bugzilla latency) Assignee
	Comment 130 • 12 years ago

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.

	Mike Habicher [:mikeh] (high bugzilla latency) Assignee
	Comment 131 • 12 years ago

Fixes the whitespace bug observed by :roc.

	Mike Habicher [:mikeh] (high bugzilla latency) Assignee
	Comment 132 • 12 years ago

Addresses most recent issues identified by :jst.

TODOs as identified in my most recent reply remain.

	Mike Habicher [:mikeh] (high bugzilla latency) Assignee
	Comment 133 • 12 years ago

Includes camera facing fix.

	Andreas Gal :gal
	Comment 134 • 12 years ago

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 =

	Mike Habicher [:mikeh] (high bugzilla latency) Assignee
	Comment 135 • 12 years ago

review ping, :jst ?

	Johnny Stenback (:jst)
	Comment 136 • 12 years ago

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.

	Boris Zbarsky [:bzbarsky]
	Comment 137 • 12 years ago

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).

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 138 • 12 years ago

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.

	Boris Zbarsky [:bzbarsky]
	Comment 139 • 12 years ago

Er, and that too.  ;)  

Fwiw, jst and I do both know jsapi....

	Boris Zbarsky [:bzbarsky]
	Comment 140 • 12 years ago

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.

	Boris Zbarsky [:bzbarsky]
	Comment 141 • 12 years ago

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.

	Boris Zbarsky [:bzbarsky]
	Comment 142 • 12 years ago

Oh, one last thing.  the JS_IsArrayObject thing in setHelper is presumably unnecessary: JS_GetArrayLength and JS_GetElement are both generic.

	Johnny Stenback (:jst)
	Comment 143 • 12 years ago

(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.

	Mike Habicher [:mikeh] (high bugzilla latency) Assignee
	Comment 144 • 12 years ago

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.

	Mike Habicher [:mikeh] (high bugzilla latency) Assignee
	Comment 145 • 12 years ago

Adds dictionaries to simplify parsing of inbound objects.

	Mike Habicher [:mikeh] (high bugzilla latency) Assignee
	Comment 146 • 12 years ago

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.

	Mike Habicher [:mikeh] (high bugzilla latency) Assignee
	Comment 147 • 12 years ago

Address :gal's last feedback.

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 148 • 12 years ago

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.

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 149 • 12 years ago

Comment on attachment 645780 [details] [diff] [review]
Part 3: new DOM framework

(Oh, and I need to touch that flag, I guess.)

	Boris Zbarsky [:bzbarsky]
	Comment 150 • 12 years ago

Comment on attachment 645780 [details] [diff] [review]
Part 3: new DOM framework

jst will handle the review...

	Mike Habicher [:mikeh] (high bugzilla latency) Assignee
	Comment 151 • 12 years ago

(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?

	Mike Habicher [:mikeh] (high bugzilla latency) Assignee
	Comment 152 • 12 years ago

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.

	Mike Habicher [:mikeh] (high bugzilla latency) Assignee
	Comment 153 • 12 years ago

Replaces interface nsICameraPictureOptions with dictionary CameraPictureOptions.

	Mike Habicher [:mikeh] (high bugzilla latency) Assignee
	Comment 154 • 12 years ago

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

	Boris Zbarsky [:bzbarsky]
	Comment 155 • 12 years ago

> 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.

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 156 • 12 years ago

(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().

	Mike Habicher [:mikeh] (high bugzilla latency) Assignee
	Comment 157 • 12 years ago

Monolithic patch rebased on m/m-c; removes NS_OUTPARAM.  No functional change intended.

	Mike Habicher [:mikeh] (high bugzilla latency) Assignee
	Comment 158 • 12 years ago

(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.

	Mike Habicher [:mikeh] (high bugzilla latency) Assignee
	Comment 159 • 12 years ago

Monolithic patch, based on feedback from :ms2ger.

	Mike Habicher [:mikeh] (high bugzilla latency) Assignee
	Comment 160 • 12 years ago

Cosmetic fixes, no functional changes intended.