-Add a mojo service to get video frames from the camera through an android service

-Add a mojo service to get video frames from the camera through an android service
-Add an example mojo app that displays this video in its view.
-Some re-organization around the camera mojo definition.

R=alhaad@google.com, qsr@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/a6ccbb4ffeb0c75d30e2b30faee2ad94e8712301

[gautham, 2015-10-02 01:20:56]

gauthamt@google.com changed reviewers:
+ alhaad@google.com

[gautham, 2015-10-02 02:03:08]

Alhaad, 

This CL also contains a patch of Tony's change to camera_roll. I'll merge once
he checks it in. 

Thanks
Gautham

[alhaad1, 2015-10-02 06:33:28]

Would love to see a demo of this :)

https://codereview.chromium.org/1375733004/diff/20001/examples/dart/device_in...
File examples/dart/device_info/lib/main.dart (left):

https://codereview.chromium.org/1375733004/diff/20001/examples/dart/device_in...
examples/dart/device_info/lib/main.dart:15: 
Stray change?

https://codereview.chromium.org/1375733004/diff/20001/mojo/services/camera/pu...
File mojo/services/camera/public/interfaces/camera.mojom (right):

https://codereview.chromium.org/1375733004/diff/20001/mojo/services/camera/pu...
mojo/services/camera/public/interfaces/camera.mojom:33: interface
CameraVideoService {
Could we call it just CameraService? There is nothing video specific about this.
We could use it to capture images as well. (And we could conceivably add finer
control later)

https://codereview.chromium.org/1375733004/diff/20001/mojo/services/camera/pu...
mojo/services/camera/public/interfaces/camera.mojom:34: GetVideoFrames() =>
(handle<data_pipe_consumer>? content);
This could be GetLatestFrame?

Also, do you think it would be better if we share the struct Photo between the
two?

https://codereview.chromium.org/1375733004/diff/20001/services/camera/BUILD.gn
File services/camera/BUILD.gn (right):

https://codereview.chromium.org/1375733004/diff/20001/services/camera/BUILD.g...
services/camera/BUILD.gn:21:
"//third_party/android_tools:google_play_services_default_java",
Do we need GMS and intent receiver now?

https://codereview.chromium.org/1375733004/diff/20001/services/camera/src/org...
File services/camera/src/org/chromium/services/camera/CameraApp.java (right):

https://codereview.chromium.org/1375733004/diff/20001/services/camera/src/org...
services/camera/src/org/chromium/services/camera/CameraApp.java:39:
CameraVideoService.MANAGER.bind(new CameraServiceImpl(mCtx, mCore, mShell),
req);
We are creating a new instance of CameraServiceImpl for every incoming
connection. This will cause us to have multiple objects trying to access the
device camera if multiple connections are made to this applications.

Instead can we have a single access to the device camera and just multiplex the
image to all the requesting incoming connection?

https://codereview.chromium.org/1375733004/diff/20001/services/camera/src/org...
File services/camera/src/org/chromium/services/camera/CameraServiceImpl.java
(right):

https://codereview.chromium.org/1375733004/diff/20001/services/camera/src/org...
services/camera/src/org/chromium/services/camera/CameraServiceImpl.java:50:
public CameraServiceImpl(Context context, Core core, Shell shell) {
The 'Shell' handle seems unused.

https://codereview.chromium.org/1375733004/diff/20001/services/camera/src/org...
services/camera/src/org/chromium/services/camera/CameraServiceImpl.java:65:
mProducerHandle.writeData(buffer, DataPipe.WriteFlags.none());
Should we check if the number of bytes written is what we expected?
Not sure if it is just better to drop such data on the floor, but then we should
set a deadline.

https://codereview.chromium.org/1375733004/diff/20001/services/camera/src/org...
services/camera/src/org/chromium/services/camera/CameraServiceImpl.java:96:
.newInstance(size.getWidth(), size.getHeight(), ImageFormat.JPEG, 30 * 600);
Just curios as to the reasoning behind 30 * 600?
Also, should we make it a file constant?

https://codereview.chromium.org/1375733004/diff/20001/shell/android/apk/Andro...
File shell/android/apk/AndroidManifest.xml.jinja2 (right):

https://codereview.chromium.org/1375733004/diff/20001/shell/android/apk/Andro...
shell/android/apk/AndroidManifest.xml.jinja2:23: <uses-permission
android:name="android.permission.WRITE_EXTERNAL_STORAGE" />
Why do we need write permission?

[qsr, 2015-10-02 08:48:30]

qsr@chromium.org changed reviewers:
+ qsr@chromium.org

[qsr, 2015-10-02 08:48:31]

https://codereview.chromium.org/1375733004/diff/20001/services/camera/src/org...
File services/camera/src/org/chromium/services/camera/CameraServiceImpl.java
(right):

https://codereview.chromium.org/1375733004/diff/20001/services/camera/src/org...
services/camera/src/org/chromium/services/camera/CameraServiceImpl.java:62: if
(mProducerHandle != null) {
mProducerHandler is accessed from multiple thread. You need synchronization.

https://codereview.chromium.org/1375733004/diff/20001/services/camera/src/org...
services/camera/src/org/chromium/services/camera/CameraServiceImpl.java:66:
mProducerHandle.close();
This close should be a in finally block somewhere, or better, the handle in a
try with resource block. You do not want to leak the handle in case of
exception.

https://codereview.chromium.org/1375733004/diff/20001/services/camera/src/org...
services/camera/src/org/chromium/services/camera/CameraServiceImpl.java:71:
Core.HandleSignals.WRITABLE, Core.DEADLINE_INFINITE);
Why this wait? You are not in a loop.

https://codereview.chromium.org/1375733004/diff/20001/services/camera/src/org...
services/camera/src/org/chromium/services/camera/CameraServiceImpl.java:126:
e.printStackTrace();
Prefer log to printStackTrace here and everywhere.

https://codereview.chromium.org/1375733004/diff/20001/services/camera/src/org...
services/camera/src/org/chromium/services/camera/CameraServiceImpl.java:132: if
(size.getWidth() == size.getHeight() * 4 / 3 && size.getWidth() <= 1080) {
Why this particular conditions?

https://codereview.chromium.org/1375733004/diff/20001/services/camera/src/org...
services/camera/src/org/chromium/services/camera/CameraServiceImpl.java:173:
Log.e(TAG, "Failed to set repeating request :" + e);
You can also pass the exception itself to the log message so that we can see the
stack, here and everywhere.

[gautham, 2015-10-02 22:29:12]

https://codereview.chromium.org/1375733004/diff/20001/examples/dart/device_in...
File examples/dart/device_info/lib/main.dart (left):

https://codereview.chromium.org/1375733004/diff/20001/examples/dart/device_in...
examples/dart/device_info/lib/main.dart:15: 
On 2015/10/02 06:33:27, alhaad1 wrote:
> Stray change?

Done.

https://codereview.chromium.org/1375733004/diff/20001/mojo/services/camera/pu...
File mojo/services/camera/public/interfaces/camera.mojom (right):

https://codereview.chromium.org/1375733004/diff/20001/mojo/services/camera/pu...
mojo/services/camera/public/interfaces/camera.mojom:33: interface
CameraVideoService {
On 2015/10/02 06:33:27, alhaad1 wrote:
> Could we call it just CameraService? There is nothing video specific about
this.
> We could use it to capture images as well. (And we could conceivably add finer
> control later) 

Done.

https://codereview.chromium.org/1375733004/diff/20001/mojo/services/camera/pu...
mojo/services/camera/public/interfaces/camera.mojom:34: GetVideoFrames() =>
(handle<data_pipe_consumer>? content);
On 2015/10/02 06:33:27, alhaad1 wrote:
> This could be GetLatestFrame?
> 
> Also, do you think it would be better if we share the struct Photo between the
> two?

I'm hoping we'll fix the ImageDecoder to work with a streaming data_pipe and
would not have not close/re-open the handle on the sender side. After that we'd
need the return value to be a data_pipe.

https://codereview.chromium.org/1375733004/diff/20001/services/camera/BUILD.gn
File services/camera/BUILD.gn (right):

https://codereview.chromium.org/1375733004/diff/20001/services/camera/BUILD.g...
services/camera/BUILD.gn:21:
"//third_party/android_tools:google_play_services_default_java",
On 2015/10/02 06:33:27, alhaad1 wrote:
> Do we need GMS and intent receiver now?

Done.

https://codereview.chromium.org/1375733004/diff/20001/services/camera/src/org...
File services/camera/src/org/chromium/services/camera/CameraApp.java (right):

https://codereview.chromium.org/1375733004/diff/20001/services/camera/src/org...
services/camera/src/org/chromium/services/camera/CameraApp.java:39:
CameraVideoService.MANAGER.bind(new CameraServiceImpl(mCtx, mCore, mShell),
req);
On 2015/10/02 06:33:27, alhaad1 wrote:
> We are creating a new instance of CameraServiceImpl for every incoming
> connection. This will cause us to have multiple objects trying to access the
> device camera if multiple connections are made to this applications.
> 
> Instead can we have a single access to the device camera and just multiplex
the
> image to all the requesting incoming connection?

I thought the shell already only opened one instance of the app. I've changed it
to create one instance of CameraServiceImpl.

https://codereview.chromium.org/1375733004/diff/20001/services/camera/src/org...
File services/camera/src/org/chromium/services/camera/CameraServiceImpl.java
(right):

https://codereview.chromium.org/1375733004/diff/20001/services/camera/src/org...
services/camera/src/org/chromium/services/camera/CameraServiceImpl.java:50:
public CameraServiceImpl(Context context, Core core, Shell shell) {
On 2015/10/02 06:33:27, alhaad1 wrote:
> The 'Shell' handle seems unused.

Done.

https://codereview.chromium.org/1375733004/diff/20001/services/camera/src/org...
services/camera/src/org/chromium/services/camera/CameraServiceImpl.java:62: if
(mProducerHandle != null) {
On 2015/10/02 08:48:31, qsr wrote:
> mProducerHandler is accessed from multiple thread. You need synchronization.

Done.

https://codereview.chromium.org/1375733004/diff/20001/services/camera/src/org...
services/camera/src/org/chromium/services/camera/CameraServiceImpl.java:65:
mProducerHandle.writeData(buffer, DataPipe.WriteFlags.none());
On 2015/10/02 06:33:27, alhaad1 wrote:
> Should we check if the number of bytes written is what we expected?
> Not sure if it is just better to drop such data on the floor, but then we
should
> set a deadline.

I did not set the ALL_OR_NONE because I figured the client can handle a failed
image decode. 
How do I set a deadline?

https://codereview.chromium.org/1375733004/diff/20001/services/camera/src/org...
services/camera/src/org/chromium/services/camera/CameraServiceImpl.java:66:
mProducerHandle.close();
On 2015/10/02 08:48:31, qsr wrote:
> This close should be a in finally block somewhere, or better, the handle in a
> try with resource block. You do not want to leak the handle in case of
> exception.

Done.

https://codereview.chromium.org/1375733004/diff/20001/services/camera/src/org...
services/camera/src/org/chromium/services/camera/CameraServiceImpl.java:71:
Core.HandleSignals.WRITABLE, Core.DEADLINE_INFINITE);
On 2015/10/02 08:48:31, qsr wrote:
> Why this wait? You are not in a loop.

Done.

https://codereview.chromium.org/1375733004/diff/20001/services/camera/src/org...
services/camera/src/org/chromium/services/camera/CameraServiceImpl.java:96:
.newInstance(size.getWidth(), size.getHeight(), ImageFormat.JPEG, 30 * 600);
On 2015/10/02 06:33:27, alhaad1 wrote:
> Just curios as to the reasoning behind 30 * 600?
> Also, should we make it a file constant?

My understanding of the acquireLatestImage vs acquireNextImage is pretty
primitive. I changed this to 100/*fps*/

https://codereview.chromium.org/1375733004/diff/20001/services/camera/src/org...
services/camera/src/org/chromium/services/camera/CameraServiceImpl.java:126:
e.printStackTrace();
On 2015/10/02 08:48:30, qsr wrote:
> Prefer log to printStackTrace here and everywhere.

Done.

https://codereview.chromium.org/1375733004/diff/20001/services/camera/src/org...
services/camera/src/org/chromium/services/camera/CameraServiceImpl.java:132: if
(size.getWidth() == size.getHeight() * 4 / 3 && size.getWidth() <= 1080) {
On 2015/10/02 08:48:31, qsr wrote:
> Why this particular conditions?

I pulled this from some other code. My understanding is that this has to do with
1080p resolution. I've changed the parameters to reflect this.

https://codereview.chromium.org/1375733004/diff/20001/services/camera/src/org...
services/camera/src/org/chromium/services/camera/CameraServiceImpl.java:173:
Log.e(TAG, "Failed to set repeating request :" + e);
On 2015/10/02 08:48:31, qsr wrote:
> You can also pass the exception itself to the log message so that we can see
the
> stack, here and everywhere.

Done.

https://codereview.chromium.org/1375733004/diff/20001/shell/android/apk/Andro...
File shell/android/apk/AndroidManifest.xml.jinja2 (right):

https://codereview.chromium.org/1375733004/diff/20001/shell/android/apk/Andro...
shell/android/apk/AndroidManifest.xml.jinja2:23: <uses-permission
android:name="android.permission.WRITE_EXTERNAL_STORAGE" />
On 2015/10/02 06:33:27, alhaad1 wrote:
> Why do we need write permission?

Done.

[alhaad1, 2015-10-05 17:40:58]

lgtm.
FYI, Tony had landed his change.

[qsr, 2015-10-05 17:48:16]

https://codereview.chromium.org/1375733004/diff/40001/services/camera/src/org...
File services/camera/src/org/chromium/services/camera/CameraApp.java (right):

https://codereview.chromium.org/1375733004/diff/40001/services/camera/src/org...
services/camera/src/org/chromium/services/camera/CameraApp.java:20: private
final Context mCtx;
Please do not use abbreviation. mContect/

Also, any reason you need those fields, there are never used.

https://codereview.chromium.org/1375733004/diff/40001/services/camera/src/org...
File services/camera/src/org/chromium/services/camera/CameraServiceImpl.java
(right):

https://codereview.chromium.org/1375733004/diff/40001/services/camera/src/org...
services/camera/src/org/chromium/services/camera/CameraServiceImpl.java:32:
import java.util.concurrent.locks.ReentrantLock;
Any reason not to use serialize instead of an explicit lock?

https://codereview.chromium.org/1375733004/diff/40001/services/camera/src/org...
services/camera/src/org/chromium/services/camera/CameraServiceImpl.java:61:
Image img = reader.acquireLatestImage();
Instead of having to close this yourself, you can use a try with resource
construct:

try (Image img = reader.acquireLatestImage()) {
  // Do whatever you need with an image.
}

https://codereview.chromium.org/1375733004/diff/40001/services/camera/src/org...
services/camera/src/org/chromium/services/camera/CameraServiceImpl.java:66:
mProducerHandle.writeData(buffer, DataPipe.WriteFlags.none());
You should save mProducerHandle to a local variable, set the field to null, and
release the lock before doing any I/O.

https://codereview.chromium.org/1375733004/diff/40001/services/camera/src/org...
services/camera/src/org/chromium/services/camera/CameraServiceImpl.java:188:
callback.call(handles.second);
What should happen if mProcuderHandler is not null?

[gautham, 2015-10-05 18:55:00]

https://codereview.chromium.org/1375733004/diff/40001/services/camera/src/org...
File services/camera/src/org/chromium/services/camera/CameraApp.java (right):

https://codereview.chromium.org/1375733004/diff/40001/services/camera/src/org...
services/camera/src/org/chromium/services/camera/CameraApp.java:20: private
final Context mCtx;
On 2015/10/05 17:48:16, qsr wrote:
> Please do not use abbreviation. mContect/
> 
> Also, any reason you need those fields, there are never used.

Done.

https://codereview.chromium.org/1375733004/diff/40001/services/camera/src/org...
File services/camera/src/org/chromium/services/camera/CameraServiceImpl.java
(right):

https://codereview.chromium.org/1375733004/diff/40001/services/camera/src/org...
services/camera/src/org/chromium/services/camera/CameraServiceImpl.java:32:
import java.util.concurrent.locks.ReentrantLock;
On 2015/10/05 17:48:16, qsr wrote:
> Any reason not to use serialize instead of an explicit lock?

Done.

https://codereview.chromium.org/1375733004/diff/40001/services/camera/src/org...
services/camera/src/org/chromium/services/camera/CameraServiceImpl.java:61:
Image img = reader.acquireLatestImage();
On 2015/10/05 17:48:16, qsr wrote:
> Instead of having to close this yourself, you can use a try with resource
> construct:
> 
> try (Image img = reader.acquireLatestImage()) {
>   // Do whatever you need with an image.
> }

Done.

https://codereview.chromium.org/1375733004/diff/40001/services/camera/src/org...
services/camera/src/org/chromium/services/camera/CameraServiceImpl.java:66:
mProducerHandle.writeData(buffer, DataPipe.WriteFlags.none());
On 2015/10/05 17:48:16, qsr wrote:
> You should save mProducerHandle to a local variable, set the field to null,
and
> release the lock before doing any I/O.

Done.

https://codereview.chromium.org/1375733004/diff/40001/services/camera/src/org...
services/camera/src/org/chromium/services/camera/CameraServiceImpl.java:188:
callback.call(handles.second);
On 2015/10/05 17:48:16, qsr wrote:
> What should happen if mProcuderHandler is not null?

Good point. I closed it.

[qsr, 2015-10-05 19:45:37]

https://codereview.chromium.org/1375733004/diff/100001/services/camera/src/or...
File services/camera/src/org/chromium/services/camera/CameraApp.java (right):

https://codereview.chromium.org/1375733004/diff/100001/services/camera/src/or...
services/camera/src/org/chromium/services/camera/CameraApp.java:34:
CameraService.MANAGER.bind(mCameraServiceImpl, req);
Hum, this seems kind of weird. You are not doing multiplexing here. Because you
service is statefull, multiple connection will step of on another, each time
someone will request an image, it will cancel the request for the previous one.

Instead you might want multiple instance of the service, that all delegate to a
single instance of a class that will handle the camera. Each time you receive an
image you will call all the currently connected service and send the data to all
of those.

[gautham, 2015-10-06 04:59:36]

-Added the TODO. Don't allow multiple applications to use the camera
simultaneously. 

-Also, had to add some synchronization to make sure openCamera and closeCamera
are handled correctly.

-We now openCamera only when an application connects to CameraApp and
closeCamera when it is done.

[qsr, 2015-10-06 16:32:26]

https://codereview.chromium.org/1375733004/diff/120001/services/camera/src/or...
File services/camera/src/org/chromium/services/camera/CameraApp.java (right):

https://codereview.chromium.org/1375733004/diff/120001/services/camera/src/or...
services/camera/src/org/chromium/services/camera/CameraApp.java:36:
mCameraServiceImpl.openCamera();
The test, and the openCamera should probably be done in the bind method. If
something connects to this application, it might never request the camera
service, which will prevent anyone to ever request it again, and something that
connects to his application might request the service multiple time, which will
be an issue in the current setup.

https://codereview.chromium.org/1375733004/diff/120001/services/camera/src/or...
File services/camera/src/org/chromium/services/camera/CameraServiceImpl.java
(right):

https://codereview.chromium.org/1375733004/diff/120001/services/camera/src/or...
services/camera/src/org/chromium/services/camera/CameraServiceImpl.java:62:
synchronized (this) {
What about doing this before acquiring the image. If there is no handle, you can
do an early return and skip the acquireLatestImage that might be costly.

https://codereview.chromium.org/1375733004/diff/120001/services/camera/src/or...
services/camera/src/org/chromium/services/camera/CameraServiceImpl.java:68:
ByteBuffer buffer = img.getPlanes()[0].getBuffer();
Can you add a todo here. Doing a single writeData is not correct, as you might
need to wait to be able to write data, and do this in a loop. This will be
particularly important if the data to write is big.

https://codereview.chromium.org/1375733004/diff/120001/services/camera/src/or...
services/camera/src/org/chromium/services/camera/CameraServiceImpl.java:233:
public synchronized boolean cameraInUse() {
Do not use synchronized method, instead us a synchronized block.

Also, you need synchronization for the handle, because you access it from
multiple threads. Why do you need more synchronization? What are the different
threads accessing the rest of the fields?

[gautham, 2015-10-06 21:31:06]

https://codereview.chromium.org/1375733004/diff/120001/services/camera/src/or...
File services/camera/src/org/chromium/services/camera/CameraApp.java (right):

https://codereview.chromium.org/1375733004/diff/120001/services/camera/src/or...
services/camera/src/org/chromium/services/camera/CameraApp.java:36:
mCameraServiceImpl.openCamera();
On 2015/10/06 16:32:26, qsr wrote:
> The test, and the openCamera should probably be done in the bind method. If
> something connects to this application, it might never request the camera
> service, which will prevent anyone to ever request it again, and something
that
> connects to his application might request the service multiple time, which
will
> be an issue in the current setup.

I'd thought of doing it in bind but I was not sure how to refuse a bind.Is it
okay to just return without binding to the implementation?. How does this get
handled?

https://codereview.chromium.org/1375733004/diff/120001/services/camera/src/or...
File services/camera/src/org/chromium/services/camera/CameraServiceImpl.java
(right):

https://codereview.chromium.org/1375733004/diff/120001/services/camera/src/or...
services/camera/src/org/chromium/services/camera/CameraServiceImpl.java:62:
synchronized (this) {
On 2015/10/06 16:32:26, qsr wrote:
> What about doing this before acquiring the image. If there is no handle, you
can
> do an early return and skip the acquireLatestImage that might be costly.

Done.

https://codereview.chromium.org/1375733004/diff/120001/services/camera/src/or...
services/camera/src/org/chromium/services/camera/CameraServiceImpl.java:68:
ByteBuffer buffer = img.getPlanes()[0].getBuffer();
On 2015/10/06 16:32:26, qsr wrote:
> Can you add a todo here. Doing a single writeData is not correct, as you might
> need to wait to be able to write data, and do this in a loop. This will be
> particularly important if the data to write is big.

Done.

https://codereview.chromium.org/1375733004/diff/120001/services/camera/src/or...
services/camera/src/org/chromium/services/camera/CameraServiceImpl.java:233:
public synchronized boolean cameraInUse() {
On 2015/10/06 16:32:26, qsr wrote:
> Do not use synchronized method, instead us a synchronized block.
> 
> Also, you need synchronization for the handle, because you access it from
> multiple threads. Why do you need more synchronization? What are the different
> threads accessing the rest of the fields?

I changed cameraInUse to only check for mImageReader and so it does not need to
be synchronized. mCameraDevice can be accessed in multiple threads and hence the
other blocks need the synchronization.

[qsr, 2015-10-06 23:13:58]

On 2015/10/06 21:31:06, gautham wrote:
>
https://codereview.chromium.org/1375733004/diff/120001/services/camera/src/or...
> File services/camera/src/org/chromium/services/camera/CameraApp.java (right):
> 
>
https://codereview.chromium.org/1375733004/diff/120001/services/camera/src/or...
> services/camera/src/org/chromium/services/camera/CameraApp.java:36:
> mCameraServiceImpl.openCamera();
> On 2015/10/06 16:32:26, qsr wrote:
> > The test, and the openCamera should probably be done in the bind method. If
> > something connects to this application, it might never request the camera
> > service, which will prevent anyone to ever request it again, and something
> that
> > connects to his application might request the service multiple time, which
> will
> > be an issue in the current setup.
> 
> I'd thought of doing it in bind but I was not sure how to refuse a bind.Is it
> okay to just return without binding to the implementation?. How does this get
> handled?

 You need to close the request, and this will be handle with whover tried to get
a service getting a connection error on the proxy it tries to create.

> 
>
https://codereview.chromium.org/1375733004/diff/120001/services/camera/src/or...
> File services/camera/src/org/chromium/services/camera/CameraServiceImpl.java
> (right):
> 
>
https://codereview.chromium.org/1375733004/diff/120001/services/camera/src/or...
> services/camera/src/org/chromium/services/camera/CameraServiceImpl.java:62:
> synchronized (this) {
> On 2015/10/06 16:32:26, qsr wrote:
> > What about doing this before acquiring the image. If there is no handle, you
> can
> > do an early return and skip the acquireLatestImage that might be costly.
> 
> Done.
> 
>
https://codereview.chromium.org/1375733004/diff/120001/services/camera/src/or...
> services/camera/src/org/chromium/services/camera/CameraServiceImpl.java:68:
> ByteBuffer buffer = img.getPlanes()[0].getBuffer();
> On 2015/10/06 16:32:26, qsr wrote:
> > Can you add a todo here. Doing a single writeData is not correct, as you
might
> > need to wait to be able to write data, and do this in a loop. This will be
> > particularly important if the data to write is big.
> 
> Done.
> 
>
https://codereview.chromium.org/1375733004/diff/120001/services/camera/src/or...
> services/camera/src/org/chromium/services/camera/CameraServiceImpl.java:233:
> public synchronized boolean cameraInUse() {
> On 2015/10/06 16:32:26, qsr wrote:
> > Do not use synchronized method, instead us a synchronized block.
> > 
> > Also, you need synchronization for the handle, because you access it from
> > multiple threads. Why do you need more synchronization? What are the
different
> > threads accessing the rest of the fields?
> 
> I changed cameraInUse to only check for mImageReader and so it does not need
to
> be synchronized. mCameraDevice can be accessed in multiple threads and hence
the
> other blocks need the synchronization.

[qsr, 2015-10-06 23:14:13]

https://codereview.chromium.org/1375733004/diff/120001/services/camera/src/or...
File services/camera/src/org/chromium/services/camera/CameraServiceImpl.java
(right):

https://codereview.chromium.org/1375733004/diff/120001/services/camera/src/or...
services/camera/src/org/chromium/services/camera/CameraServiceImpl.java:233:
public synchronized boolean cameraInUse() {
On 2015/10/06 21:31:06, gautham wrote:
> On 2015/10/06 16:32:26, qsr wrote:
> > Do not use synchronized method, instead us a synchronized block.
> > 
> > Also, you need synchronization for the handle, because you access it from
> > multiple threads. Why do you need more synchronization? What are the
different
> > threads accessing the rest of the fields?
> 
> I changed cameraInUse to only check for mImageReader and so it does not need
to
> be synchronized. mCameraDevice can be accessed in multiple threads and hence
the
> other blocks need the synchronization. 

I don't understand this. On which thread can mCameraDevice be accessed? The only
synchronized block you still have is on close() and open camera, and both should
always called on the execution thread of this service.

https://codereview.chromium.org/1375733004/diff/180001/services/camera/src/or...
File services/camera/src/org/chromium/services/camera/CameraServiceImpl.java
(right):

https://codereview.chromium.org/1375733004/diff/180001/services/camera/src/or...
services/camera/src/org/chromium/services/camera/CameraServiceImpl.java:69: try
{
I think you do not need the second try, you can use catch and finally with the
try with resource block.

[gautham, 2015-10-06 23:37:18]

-Moved the cameraInUse check to the bind block. 

-mCameraDevice: Your right in that mImageReader does not need to be protected. I
removed this. The synchronization with mCameraOpenCloseLock is to make sure that
closeCamera does not get called before callbacks from openCamera finish (
mCameraStateCallback/startPreview use mCameraDevice/mImageReader)

https://codereview.chromium.org/1375733004/diff/180001/services/camera/src/or...
File services/camera/src/org/chromium/services/camera/CameraServiceImpl.java
(right):

https://codereview.chromium.org/1375733004/diff/180001/services/camera/src/or...
services/camera/src/org/chromium/services/camera/CameraServiceImpl.java:69: try
{
On 2015/10/06 23:14:12, qsr wrote:
> I think you do not need the second try, you can use catch and finally with the
> try with resource block.

Done.

[qsr, 2015-10-07 15:45:51]

https://codereview.chromium.org/1375733004/diff/220001/services/camera/src/or...
File services/camera/src/org/chromium/services/camera/CameraApp.java (right):

https://codereview.chromium.org/1375733004/diff/220001/services/camera/src/or...
services/camera/src/org/chromium/services/camera/CameraApp.java:33: public void
bind(InterfaceRequest<CameraService> req) {
s/req/request/

We do not use abbreviation.

https://codereview.chromium.org/1375733004/diff/220001/services/camera/src/or...
File services/camera/src/org/chromium/services/camera/CameraServiceImpl.java
(right):

https://codereview.chromium.org/1375733004/diff/220001/services/camera/src/or...
services/camera/src/org/chromium/services/camera/CameraServiceImpl.java:42:
private Semaphore mCameraOpenCloseLock = new Semaphore(1);
Why do you use a Semaphore, and not a lock? A lock also has the advantage that
you won't need to handle interuptedException.

https://codereview.chromium.org/1375733004/diff/220001/services/camera/src/or...
services/camera/src/org/chromium/services/camera/CameraServiceImpl.java:99:
manager.openCamera(cameraId, mCameraStateCallback, mHandler);
If openCamera throws an exception, you will never release your lock. You should
move the lock acquire just before this method, and have a try block around so
you can release the lock if it throws.

https://codereview.chromium.org/1375733004/diff/220001/services/camera/src/or...
services/camera/src/org/chromium/services/camera/CameraServiceImpl.java:122: }
finally {
This will be called if acquire throws an Interrupted exception and you will try
to release a semaphore you did not acquire. You need something like:

try {
  mCameraOpenCloseLock.acquire();
  try {
    ...
  } finally {
    mCameraOpenCloseLock.release();
  }
} catch (InterruptedException e) {
  throw new RuntimeException("Interrupted while trying to lock camera
opening.");
}

[gautham, 2015-10-07 18:31:41]

https://codereview.chromium.org/1375733004/diff/220001/services/camera/src/or...
File services/camera/src/org/chromium/services/camera/CameraApp.java (right):

https://codereview.chromium.org/1375733004/diff/220001/services/camera/src/or...
services/camera/src/org/chromium/services/camera/CameraApp.java:33: public void
bind(InterfaceRequest<CameraService> req) {
On 2015/10/07 15:45:51, qsr wrote:
> s/req/request/
> 
> We do not use abbreviation.

Done.

https://codereview.chromium.org/1375733004/diff/220001/services/camera/src/or...
File services/camera/src/org/chromium/services/camera/CameraServiceImpl.java
(right):

https://codereview.chromium.org/1375733004/diff/220001/services/camera/src/or...
services/camera/src/org/chromium/services/camera/CameraServiceImpl.java:42:
private Semaphore mCameraOpenCloseLock = new Semaphore(1);
On 2015/10/07 15:45:51, qsr wrote:
> Why do you use a Semaphore, and not a lock? A lock also has the advantage that
> you won't need to handle interuptedException.

We cannot lock/unlock (java.util.concurrent.locks.ReentrantLock) from different
threads.

https://codereview.chromium.org/1375733004/diff/220001/services/camera/src/or...
services/camera/src/org/chromium/services/camera/CameraServiceImpl.java:99:
manager.openCamera(cameraId, mCameraStateCallback, mHandler);
On 2015/10/07 15:45:51, qsr wrote:
> If openCamera throws an exception, you will never release your lock. You
should
> move the lock acquire just before this method, and have a try block around so
> you can release the lock if it throws.

Done.

[qsr, 2015-10-07 18:46:32]

LGTM with nits.

https://codereview.chromium.org/1375733004/diff/240001/services/camera/src/or...
File services/camera/src/org/chromium/services/camera/CameraServiceImpl.java
(right):

https://codereview.chromium.org/1375733004/diff/240001/services/camera/src/or...
services/camera/src/org/chromium/services/camera/CameraServiceImpl.java:125:
mCameraOpenCloseLock.release();
This should be in a finally block.

[gautham, 2015-10-07 19:36:53]

Thanks for the detailed reviews.

https://codereview.chromium.org/1375733004/diff/240001/services/camera/src/or...
File services/camera/src/org/chromium/services/camera/CameraServiceImpl.java
(right):

https://codereview.chromium.org/1375733004/diff/240001/services/camera/src/or...
services/camera/src/org/chromium/services/camera/CameraServiceImpl.java:125:
mCameraOpenCloseLock.release();
On 2015/10/07 18:46:32, qsr wrote:
> This should be in a finally block.

Done.

[gautham, 2015-10-07 20:43:46]

Committed patchset #15 (id:280001) manually as
a6ccbb4ffeb0c75d30e2b30faee2ad94e8712301 (presubmit successful).