Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1178703009:
VideoCapturerAndroid: Add function to change capture format while camera is running (Closed)
|
Created:
5 years, 6 months ago by magjed_webrtc Modified:
5 years, 5 months ago CC:
webrtc-reviews_webrtc.org, interface-changes_webrtc.org, rwolff_gocast.it, yujie_mao (webrtc), Andrew MacDonald, tterriberry_mozilla.com, qiang.lu, niklas.enbom, åsapersson Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionVideoCapturerAndroid: Add function to change capture format while camera is running
Committed: https://crrev.com/b69ab79338bff71ea411b82f3dd59508617a11d5
Cr-Commit-Position: refs/heads/master@{#9608}
Patch Set 1 #
Total comments: 9
Patch Set 2 : hbos@ comments #
Total comments: 4
Patch Set 3 : wip #Patch Set 4 : hbos@ comments2 #Patch Set 5 : #
Total comments: 14
Patch Set 6 : glaznev@s comments #Patch Set 7 : glaznev@s comments #2 #Patch Set 8 : glaznev@s comments #3 #Patch Set 9 : Refactor #
Total comments: 12
Patch Set 10 : rebase and addressing comments #
Messages
Total messages: 38 (11 generated)
The CQ bit was checked by magjed@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1178703009/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No LGTM from a valid reviewer yet. Only full committers are accepted. Even if an LGTM may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
magjed@webrtc.org changed reviewers: + hbos@webrtc.org
hbos - Can you take a first look?
https://codereview.webrtc.org/1178703009/diff/1/talk/app/webrtc/java/src/org/... File talk/app/webrtc/java/src/org/webrtc/VideoCapturerAndroid.java (right): https://codereview.webrtc.org/1178703009/diff/1/talk/app/webrtc/java/src/org/... talk/app/webrtc/java/src/org/webrtc/VideoCapturerAndroid.java:593: private void changeCaptureFormatOnCameraThread(int width, int height, int framerate) { What if the capturer is currently not running? Will this start capturing? Possibly not properly initialized? https://codereview.webrtc.org/1178703009/diff/1/talk/app/webrtc/java/src/org/... talk/app/webrtc/java/src/org/webrtc/VideoCapturerAndroid.java:775: Log.w(TAG, "reserveByteBuffer failed - dropping frame"); cameraFramesCount-- or no? https://codereview.webrtc.org/1178703009/diff/1/talk/app/webrtc/java/src/org/... talk/app/webrtc/java/src/org/webrtc/VideoCapturerAndroid.java:895: cameraFrames.remove(frame); Ought we not re-queueCameraBuffers or the equivalent? Could we start dropping buffers and then run out of buffers? https://codereview.webrtc.org/1178703009/diff/1/webrtc/base/bind.h File webrtc/base/bind.h (right): https://codereview.webrtc.org/1178703009/diff/1/webrtc/base/bind.h#newcode582 webrtc/base/bind.h:582: mrooe
https://codereview.webrtc.org/1178703009/diff/1/talk/app/webrtc/java/src/org/... File talk/app/webrtc/java/src/org/webrtc/VideoCapturerAndroid.java (right): https://codereview.webrtc.org/1178703009/diff/1/talk/app/webrtc/java/src/org/... talk/app/webrtc/java/src/org/webrtc/VideoCapturerAndroid.java:593: private void changeCaptureFormatOnCameraThread(int width, int height, int framerate) { On 2015/06/12 09:34:48, hbos wrote: > What if the capturer is currently not running? Will this start capturing? > Possibly not properly initialized? Done. Added checks. https://codereview.webrtc.org/1178703009/diff/1/talk/app/webrtc/java/src/org/... talk/app/webrtc/java/src/org/webrtc/VideoCapturerAndroid.java:775: Log.w(TAG, "reserveByteBuffer failed - dropping frame"); On 2015/06/12 09:34:48, hbos wrote: > cameraFramesCount-- or no? Done. Moved 'cameraFramesCount++' into the same scope as frameObserver.OnFrameCaptured. https://codereview.webrtc.org/1178703009/diff/1/talk/app/webrtc/java/src/org/... talk/app/webrtc/java/src/org/webrtc/VideoCapturerAndroid.java:895: cameraFrames.remove(frame); On 2015/06/12 09:34:48, hbos wrote: > Ought we not re-queueCameraBuffers or the equivalent? Could we start dropping > buffers and then run out of buffers? No, that shouldn't be possible. The function queueCameraBuffer is responsible for (re)queuing buffers. If frameSize changes, we always add |numCaptureBuffersAvailable| new buffers.
https://codereview.webrtc.org/1178703009/diff/1/talk/app/webrtc/java/src/org/... File talk/app/webrtc/java/src/org/webrtc/VideoCapturerAndroid.java (right): https://codereview.webrtc.org/1178703009/diff/1/talk/app/webrtc/java/src/org/... talk/app/webrtc/java/src/org/webrtc/VideoCapturerAndroid.java:593: private void changeCaptureFormatOnCameraThread(int width, int height, int framerate) { On 2015/06/12 12:52:23, magjed_webrtc wrote: > On 2015/06/12 09:34:48, hbos wrote: > > What if the capturer is currently not running? Will this start capturing? > > Possibly not properly initialized? > > Done. Added checks. Add a comment to changeCaptureFormat that it can only be called whilst running or else its a no-op. https://codereview.webrtc.org/1178703009/diff/20001/talk/app/webrtc/java/src/... File talk/app/webrtc/java/src/org/webrtc/VideoCapturerAndroid.java (right): https://codereview.webrtc.org/1178703009/diff/20001/talk/app/webrtc/java/src/... talk/app/webrtc/java/src/org/webrtc/VideoCapturerAndroid.java:277: public synchronized void changeCaptureFormat( Speaking of synchronized, this can be called from any thread but cameraThreadHandler can be set to null in startCaptureOnCameraThread, which does not use synchronized (perhaps it should?). https://codereview.webrtc.org/1178703009/diff/20001/talk/app/webrtc/java/src/... talk/app/webrtc/java/src/org/webrtc/VideoCapturerAndroid.java:279: cameraThreadHandler.post(new Runnable() { Also, even if safely synchronized, cameraThreadHandler might already be null and it would crash and not log.
PTAL https://codereview.webrtc.org/1178703009/diff/1/talk/app/webrtc/java/src/org/... File talk/app/webrtc/java/src/org/webrtc/VideoCapturerAndroid.java (right): https://codereview.webrtc.org/1178703009/diff/1/talk/app/webrtc/java/src/org/... talk/app/webrtc/java/src/org/webrtc/VideoCapturerAndroid.java:593: private void changeCaptureFormatOnCameraThread(int width, int height, int framerate) { On 2015/06/15 07:42:12, hbos wrote: > On 2015/06/12 12:52:23, magjed_webrtc wrote: > > On 2015/06/12 09:34:48, hbos wrote: > > > What if the capturer is currently not running? Will this start capturing? > > > Possibly not properly initialized? > > > > Done. Added checks. > > Add a comment to changeCaptureFormat that it can only be called whilst running > or else its a no-op. Done. https://codereview.webrtc.org/1178703009/diff/20001/talk/app/webrtc/java/src/... File talk/app/webrtc/java/src/org/webrtc/VideoCapturerAndroid.java (right): https://codereview.webrtc.org/1178703009/diff/20001/talk/app/webrtc/java/src/... talk/app/webrtc/java/src/org/webrtc/VideoCapturerAndroid.java:277: public synchronized void changeCaptureFormat( On 2015/06/15 07:42:12, hbos wrote: > Speaking of synchronized, this can be called from any thread but > cameraThreadHandler can be set to null in startCaptureOnCameraThread, which does > not use synchronized (perhaps it should?). Yes, this sounds like a potential problem, not just in my function. We should fix it in a separate CL. https://codereview.webrtc.org/1178703009/diff/20001/talk/app/webrtc/java/src/... talk/app/webrtc/java/src/org/webrtc/VideoCapturerAndroid.java:279: cameraThreadHandler.post(new Runnable() { On 2015/06/15 07:42:12, hbos wrote: > Also, even if safely synchronized, cameraThreadHandler might already be null and > it would crash and not log. Done.
lgtm
magjed@webrtc.org changed reviewers: + glaznev@webrtc.org
glaznev - Please take a look.
On 2015/06/18 13:48:09, magjed_webrtc wrote: > glaznev - Please take a look. +Asa. Asa recently submitted a change with allows SW scaling of camera frames https://codereview.webrtc.org/1178643006/ This CL adds an option to change camera resolution by restarting the preview. Seems like on of these 2 changes is not necessary - or I may be wrong?
https://codereview.webrtc.org/1178703009/diff/80001/talk/app/webrtc/java/src/... File talk/app/webrtc/java/src/org/webrtc/VideoCapturerAndroid.java (right): https://codereview.webrtc.org/1178703009/diff/80001/talk/app/webrtc/java/src/... talk/app/webrtc/java/src/org/webrtc/VideoCapturerAndroid.java:932: if (frame.frameSize != frameSize) { Please add a comment how this condition can happen - when you call videoBuffers.queueCameraBuffers() with new width and height you are creating 3 new camera frames with new sizes - so cameraFrames array will always contain frames with updates sizes. What can cause frame.frameSize != frameSize?
I have spoken with Åsa about https://codereview.webrtc.org/1178643006/. They are indeed very similar. The advantage with restarting the preview is that we save more CPU/battery vs SW scaling, and the drawback is that we won't receive any frames for ~1s while restarting. If this CL lands, we can add some logic in onOutputFormatRequest when it's worth to restart the preview (hysteresis and/or not too often), but I would like to do that in a separate CL. Also, I'm implementing a capture quality slider where I want to change actual capture format, without any filtering between. https://codereview.webrtc.org/1178703009/diff/80001/talk/app/webrtc/java/src/... File talk/app/webrtc/java/src/org/webrtc/VideoCapturerAndroid.java (right): https://codereview.webrtc.org/1178703009/diff/80001/talk/app/webrtc/java/src/... talk/app/webrtc/java/src/org/webrtc/VideoCapturerAndroid.java:932: if (frame.frameSize != frameSize) { On 2015/06/18 19:51:27, AlexG wrote: > Please add a comment how this condition can happen - when you call > videoBuffers.queueCameraBuffers() with new width and height you are creating 3 > new camera frames with new sizes - so cameraFrames array will always contain > frames with updates sizes. What can cause frame.frameSize != frame We don't clear |cameraFrames| so it may still contain old frame sizes. The buffers are added to the camera preview callback buffer queue with 'camera.addCallbackBuffer' and apparently the camera may call onPreviewFrame with an outdated buffer.
https://codereview.webrtc.org/1178703009/diff/80001/talk/app/webrtc/java/src/... File talk/app/webrtc/java/src/org/webrtc/VideoCapturerAndroid.java (right): https://codereview.webrtc.org/1178703009/diff/80001/talk/app/webrtc/java/src/... talk/app/webrtc/java/src/org/webrtc/VideoCapturerAndroid.java:875: boolean cameraWasNull = (this.camera == null); Why not to throw exception here? I am not seeing any changes at a first glance which may call queueCameraBuffers for already closed camera. https://codereview.webrtc.org/1178703009/diff/80001/talk/app/webrtc/java/src/... talk/app/webrtc/java/src/org/webrtc/VideoCapturerAndroid.java:889: } else if (cameraWasNull) { Camera is null, but you are still calling camera.addCallbackBuffer() a few lines below? https://codereview.webrtc.org/1178703009/diff/80001/talk/app/webrtc/java/src/... talk/app/webrtc/java/src/org/webrtc/VideoCapturerAndroid.java:898: // Same framesize and camera is still running - no need to do anything. Probably this check is better to do in changeCaptureFormat() call and ignore capture change format request if actual format does not change. https://codereview.webrtc.org/1178703009/diff/80001/talk/app/webrtc/java/src/... talk/app/webrtc/java/src/org/webrtc/VideoCapturerAndroid.java:901: Log.d(TAG, "queueCameraBuffers enqued " + numCaptureBuffersAvailable Probably also worth to log total amount of byte buffers in |cameraFrames| now - both old and new sizes.
https://codereview.webrtc.org/1178703009/diff/80001/talk/app/webrtc/java/src/... File talk/app/webrtc/java/src/org/webrtc/VideoCapturerAndroid.java (right): https://codereview.webrtc.org/1178703009/diff/80001/talk/app/webrtc/java/src/... talk/app/webrtc/java/src/org/webrtc/VideoCapturerAndroid.java:875: boolean cameraWasNull = (this.camera == null); On 2015/06/22 17:27:40, AlexG wrote: > Why not to throw exception here? I am not seeing any changes at a first glance > which may call queueCameraBuffers for already closed camera. The new function changeCaptureFormatOnCameraThread calls it, and then this.camera != null. https://codereview.webrtc.org/1178703009/diff/80001/talk/app/webrtc/java/src/... talk/app/webrtc/java/src/org/webrtc/VideoCapturerAndroid.java:889: } else if (cameraWasNull) { On 2015/06/22 17:27:40, AlexG wrote: > Camera is null, but you are still calling camera.addCallbackBuffer() a few lines > below? The camera was null (this.camera), so that's why I have to requeue the buffers. The input argument |camera| is never null. https://codereview.webrtc.org/1178703009/diff/80001/talk/app/webrtc/java/src/... talk/app/webrtc/java/src/org/webrtc/VideoCapturerAndroid.java:898: // Same framesize and camera is still running - no need to do anything. On 2015/06/22 17:27:40, AlexG wrote: > Probably this check is better to do in changeCaptureFormat() call and ignore > capture change format request if actual format does not change. Done.
https://codereview.webrtc.org/1178703009/diff/80001/talk/app/webrtc/java/src/... File talk/app/webrtc/java/src/org/webrtc/VideoCapturerAndroid.java (right): https://codereview.webrtc.org/1178703009/diff/80001/talk/app/webrtc/java/src/... talk/app/webrtc/java/src/org/webrtc/VideoCapturerAndroid.java:875: boolean cameraWasNull = (this.camera == null); On 2015/06/22 21:39:19, magjed_webrtc wrote: > On 2015/06/22 17:27:40, AlexG wrote: > > Why not to throw exception here? I am not seeing any changes at a first glance > > which may call queueCameraBuffers for already closed camera. > > The new function changeCaptureFormatOnCameraThread calls it, and then > this.camera != null. Why not to call stopReturnBuffersToCamera() then in changeCaptureFormatOnCameraThread() similar to regular camera start()/stop() sequence? FramePool is already looks complicated - if adding more branching to it can be avoided it is better to do it. https://codereview.webrtc.org/1178703009/diff/80001/talk/app/webrtc/java/src/... talk/app/webrtc/java/src/org/webrtc/VideoCapturerAndroid.java:932: if (frame.frameSize != frameSize) { On 2015/06/22 10:32:27, magjed_webrtc wrote: > On 2015/06/18 19:51:27, AlexG wrote: > > Please add a comment how this condition can happen - when you call > > videoBuffers.queueCameraBuffers() with new width and height you are creating 3 > > new camera frames with new sizes - so cameraFrames array will always contain > > frames with updates sizes. What can cause frame.frameSize != frame > We don't clear |cameraFrames| so it may still contain old frame sizes. The > buffers are added to the camera preview callback buffer queue with > 'camera.addCallbackBuffer' and apparently the camera may call onPreviewFrame > with an outdated buffer. This is already handled in returnBuffer() call. Why do we need separate processing here instead of doing it in returnBuffer()?
https://codereview.webrtc.org/1178703009/diff/80001/talk/app/webrtc/java/src/... File talk/app/webrtc/java/src/org/webrtc/VideoCapturerAndroid.java (right): https://codereview.webrtc.org/1178703009/diff/80001/talk/app/webrtc/java/src/... talk/app/webrtc/java/src/org/webrtc/VideoCapturerAndroid.java:875: boolean cameraWasNull = (this.camera == null); On 2015/06/22 22:28:08, AlexG wrote: > On 2015/06/22 21:39:19, magjed_webrtc wrote: > > On 2015/06/22 17:27:40, AlexG wrote: > > > Why not to throw exception here? I am not seeing any changes at a first > glance > > > which may call queueCameraBuffers for already closed camera. > > > > The new function changeCaptureFormatOnCameraThread calls it, and then > > this.camera != null. > > Why not to call stopReturnBuffersToCamera() then in > changeCaptureFormatOnCameraThread() similar to regular camera start()/stop() > sequence? FramePool is already looks complicated - if adding more branching to > it can be avoided it is better to do it. Because we don't want to stop returning buffers when just changing formats. I removed that branching. https://codereview.webrtc.org/1178703009/diff/80001/talk/app/webrtc/java/src/... talk/app/webrtc/java/src/org/webrtc/VideoCapturerAndroid.java:901: Log.d(TAG, "queueCameraBuffers enqued " + numCaptureBuffersAvailable On 2015/06/22 17:27:40, AlexG wrote: > Probably also worth to log total amount of byte buffers in |cameraFrames| now - > both old and new sizes. Done. https://codereview.webrtc.org/1178703009/diff/80001/talk/app/webrtc/java/src/... talk/app/webrtc/java/src/org/webrtc/VideoCapturerAndroid.java:932: if (frame.frameSize != frameSize) { On 2015/06/22 22:28:08, AlexG wrote: > On 2015/06/22 10:32:27, magjed_webrtc wrote: > > On 2015/06/18 19:51:27, AlexG wrote: > > > Please add a comment how this condition can happen - when you call > > > videoBuffers.queueCameraBuffers() with new width and height you are creating > 3 > > > new camera frames with new sizes - so cameraFrames array will always contain > > > frames with updates sizes. What can cause frame.frameSize != frame > > We don't clear |cameraFrames| so it may still contain old frame sizes. The > > buffers are added to the camera preview callback buffer queue with > > 'camera.addCallbackBuffer' and apparently the camera may call onPreviewFrame > > with an outdated buffer. > > This is already handled in returnBuffer() call. Why do we need separate > processing here instead of doing it in returnBuffer()? returnBuffer handles the case when the format changed during the time the frame was in flight. Frames that have been sent but not returned are still valid, but we can't add them back to the queue. This check makes sure we don't send frames that are invalid. The camera has an internal queue with buffers, with correct frame size at the time of creation, but not necessarily anymore.
On 2015/06/23 16:29:17, magjed_webrtc wrote: > https://codereview.webrtc.org/1178703009/diff/80001/talk/app/webrtc/java/src/... > File talk/app/webrtc/java/src/org/webrtc/VideoCapturerAndroid.java (right): > > https://codereview.webrtc.org/1178703009/diff/80001/talk/app/webrtc/java/src/... > talk/app/webrtc/java/src/org/webrtc/VideoCapturerAndroid.java:875: boolean > cameraWasNull = (this.camera == null); > On 2015/06/22 22:28:08, AlexG wrote: > > On 2015/06/22 21:39:19, magjed_webrtc wrote: > > > On 2015/06/22 17:27:40, AlexG wrote: > > > > Why not to throw exception here? I am not seeing any changes at a first > > glance > > > > which may call queueCameraBuffers for already closed camera. > > > > > > The new function changeCaptureFormatOnCameraThread calls it, and then > > > this.camera != null. > > > > Why not to call stopReturnBuffersToCamera() then in > > changeCaptureFormatOnCameraThread() similar to regular camera start()/stop() > > sequence? FramePool is already looks complicated - if adding more branching > to > > it can be avoided it is better to do it. > > Because we don't want to stop returning buffers when just changing formats. I > removed that branching. > > https://codereview.webrtc.org/1178703009/diff/80001/talk/app/webrtc/java/src/... > talk/app/webrtc/java/src/org/webrtc/VideoCapturerAndroid.java:901: Log.d(TAG, > "queueCameraBuffers enqued " + numCaptureBuffersAvailable > On 2015/06/22 17:27:40, AlexG wrote: > > Probably also worth to log total amount of byte buffers in |cameraFrames| now > - > > both old and new sizes. > > Done. > > https://codereview.webrtc.org/1178703009/diff/80001/talk/app/webrtc/java/src/... > talk/app/webrtc/java/src/org/webrtc/VideoCapturerAndroid.java:932: if > (frame.frameSize != frameSize) { > On 2015/06/22 22:28:08, AlexG wrote: > > On 2015/06/22 10:32:27, magjed_webrtc wrote: > > > On 2015/06/18 19:51:27, AlexG wrote: > > > > Please add a comment how this condition can happen - when you call > > > > videoBuffers.queueCameraBuffers() with new width and height you are > creating > > 3 > > > > new camera frames with new sizes - so cameraFrames array will always > contain > > > > frames with updates sizes. What can cause frame.frameSize != frame > > > We don't clear |cameraFrames| so it may still contain old frame sizes. The > > > buffers are added to the camera preview callback buffer queue with > > > 'camera.addCallbackBuffer' and apparently the camera may call onPreviewFrame > > > with an outdated buffer. > > > > This is already handled in returnBuffer() call. Why do we need separate > > processing here instead of doing it in returnBuffer()? > > returnBuffer handles the case when the format changed during the time the frame > was in flight. Frames that have been sent but not returned are still valid, but > we can't add them back to the queue. This check makes sure we don't send frames > that are invalid. The camera has an internal queue with buffers, with correct > frame size at the time of creation, but not necessarily anymore. Magnus, sorry - I am trying to understand the need of adding more logic to the way frames are handled. Can you please elaborate a little more on it. My understanding is that you can change the format similar to camera switching - i.e. by calling doStopCaptureOnCameraThread(); then update width, height and fps values and then call startCaptureOnCameraThread() and current FramePool implementation will handle this case - right? Now as I understand you want to remove the overhead of camera.release() and camera.open() calls and wrote changeCaptureFormatOnCameraThread() which will actually similar to doStopCaptureOnCameraThread() -> startCaptureOnCameraThread() sequence but avoiding reopening camera / recreating camera surface. But why is it necessary to change frame handling logic then? Is it because you omitted videoBuffers.stopReturnBuffersToCamera() in you format change sequence or is it because current frame size change logic is broken and if you implement format change as doStopCaptureOnCameraThread() -> change format -> startCaptureOnCameraThread() then current implementation will not work?
Patchset #9 (id:160001) has been deleted
Patchset #9 (id:180001) has been deleted
Yes, you can change capture format like this:
cameraThreadHandler.post(new Runnable() {
@Override public void run() {
doStopCaptureOnCameraThread();
VideoCapturerAndroid.this.width = width;
VideoCapturerAndroid.this.height = height;
VideoCapturerAndroid.this.framerate = framerate;
startCaptureOnCameraThread(width, height, framerate, frameObserver,
applicationContext);
}
});
and the main reason for not implementing it like that is the overhead of
recreating the camera. But it is also not working correctly.
The current FramePool has logic to handle format change, but it's broken. If I
use the implementation above it will crash in about 5% of the cases in
reserveByteBuffer with exception "unknown data buffer?!?". It's also leaking
buffers. We only remove frames from |cameraFrames| in one rare path in
|returnBuffer|. If we change capture format, the old buffers will just idle in
|cameraFrames| and never be freed.
I have spent some time refactoring this now, mainly the FramePool. I have done
the following changes:
* Merge changeCaptureFormatOnCameraThread with the code in
startCaptureOnCameraThread.
* Remove the class FramePool.Frame, only ByteBuffer is needed.
* Separate |cameraFrames| into two HashMaps - one for queued frames, and one
for pending frames. This separation was previously done with 'timeStamp < 0'
checks.
* I don't set picture size anymore. We don't take pictures, so I don't know why
we have this code.
* Convert getPictureSize to getPreviewSize instead, for finding closest
supported resolution.
* If switchCamera can't use the exact same resolution for the other camera, it
will take the closest one instead of aborting.
Ping
lgtm Much cleaner now! Thanks! https://codereview.webrtc.org/1178703009/diff/200001/talk/app/webrtc/java/src... File talk/app/webrtc/java/src/org/webrtc/VideoCapturerAndroid.java (right): https://codereview.webrtc.org/1178703009/diff/200001/talk/app/webrtc/java/src... talk/app/webrtc/java/src/org/webrtc/VideoCapturerAndroid.java:408: public boolean equals(Object that) { nit: for clarity check if that == null and return false? This may happen in captureFormat.equals(this.captureFormat) call in startPreviewOnCameraThread() https://codereview.webrtc.org/1178703009/diff/200001/talk/app/webrtc/java/src... talk/app/webrtc/java/src/org/webrtc/VideoCapturerAndroid.java:596: final CaptureFormat captureFormat = new CaptureFormat( I think it is worth to add one more parameter to CaptureFormat - requestedFramerate (captureFormat.requestedFramerate = framerate) and use it instead of averageFps in switchCamera() call. https://codereview.webrtc.org/1178703009/diff/200001/talk/app/webrtc/java/src... talk/app/webrtc/java/src/org/webrtc/VideoCapturerAndroid.java:618: parameters.setPreviewSize(captureFormat.width, captureFormat.height); setPictureSize() was called to workaround aspect ratio problem on N7 - https://code.google.com/p/webrtc/issues/detail?id=3971. I think it is better to return this call back. https://codereview.webrtc.org/1178703009/diff/200001/talk/app/webrtc/java/src... talk/app/webrtc/java/src/org/webrtc/VideoCapturerAndroid.java:623: camera.stopPreview(); Is it safe to call stopPreview() if startPreview() was not called before? https://codereview.webrtc.org/1178703009/diff/200001/talk/app/webrtc/java/src... talk/app/webrtc/java/src/org/webrtc/VideoCapturerAndroid.java:688: final int width = captureFormat.width; nit: final is not needed here https://codereview.webrtc.org/1178703009/diff/200001/talk/app/webrtc/java/src... talk/app/webrtc/java/src/org/webrtc/VideoCapturerAndroid.java:691: final int averageFps = (captureFormat.minFramerate + captureFormat.maxFramerate) / 2000; Add requestedFps to CaptureFormat?
Patchset #10 (id:220001) has been deleted
The CQ bit was checked by magjed@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1178703009/240001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.webrtc.org/1178703009/diff/200001/talk/app/webrtc/java/src... File talk/app/webrtc/java/src/org/webrtc/VideoCapturerAndroid.java (right): https://codereview.webrtc.org/1178703009/diff/200001/talk/app/webrtc/java/src... talk/app/webrtc/java/src/org/webrtc/VideoCapturerAndroid.java:408: public boolean equals(Object that) { On 2015/07/21 00:17:08, AlexG wrote: > nit: for clarity check if that == null and return false? This may happen in > captureFormat.equals(this.captureFormat) call in startPreviewOnCameraThread() It's not really needed, because "null instanceof CaptureFormat" will evaluate to false. Do you want me to add the check anyway? https://codereview.webrtc.org/1178703009/diff/200001/talk/app/webrtc/java/src... talk/app/webrtc/java/src/org/webrtc/VideoCapturerAndroid.java:596: final CaptureFormat captureFormat = new CaptureFormat( On 2015/07/21 00:17:08, AlexG wrote: > I think it is worth to add one more parameter to CaptureFormat - > requestedFramerate (captureFormat.requestedFramerate = framerate) and use it > instead of averageFps in switchCamera() call. I added requestedFramerate as a separate parameter in the capturer class, along with requestedWidth/Height to replace averageFps etc in switchCamera() call. I don't want to add it to CaptureFormat, because a requestedFramerate variable does not make sense in e.g. supportedFormats. https://codereview.webrtc.org/1178703009/diff/200001/talk/app/webrtc/java/src... talk/app/webrtc/java/src/org/webrtc/VideoCapturerAndroid.java:618: parameters.setPreviewSize(captureFormat.width, captureFormat.height); On 2015/07/21 00:17:08, AlexG wrote: > setPictureSize() was called to workaround aspect ratio problem on N7 - > https://code.google.com/p/webrtc/issues/detail?id=3971. I think it is better to > return this call back. I see, I added it back along with a comment to give some context. https://codereview.webrtc.org/1178703009/diff/200001/talk/app/webrtc/java/src... talk/app/webrtc/java/src/org/webrtc/VideoCapturerAndroid.java:623: camera.stopPreview(); On 2015/07/21 00:17:08, AlexG wrote: > Is it safe to call stopPreview() if startPreview() was not called before? I guess, but the documentation doesn't mention it. To be on the safe side, I added a check so that we only call stopPreview if we are currently running. https://codereview.webrtc.org/1178703009/diff/200001/talk/app/webrtc/java/src... talk/app/webrtc/java/src/org/webrtc/VideoCapturerAndroid.java:688: final int width = captureFormat.width; On 2015/07/21 00:17:08, AlexG wrote: > nit: final is not needed here I like final, but this is gone now anyway.
lgtm https://codereview.webrtc.org/1178703009/diff/200001/talk/app/webrtc/java/src... File talk/app/webrtc/java/src/org/webrtc/VideoCapturerAndroid.java (right): https://codereview.webrtc.org/1178703009/diff/200001/talk/app/webrtc/java/src... talk/app/webrtc/java/src/org/webrtc/VideoCapturerAndroid.java:408: public boolean equals(Object that) { On 2015/07/21 17:20:32, magjed_webrtc wrote: > On 2015/07/21 00:17:08, AlexG wrote: > > nit: for clarity check if that == null and return false? This may happen in > > captureFormat.equals(this.captureFormat) call in startPreviewOnCameraThread() > > It's not really needed, because "null instanceof CaptureFormat" will evaluate to > false. Do you want me to add the check anyway? Didn't know that. Then check is not needed indeed
The CQ bit was checked by magjed@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from hbos@webrtc.org Link to the patchset: https://codereview.webrtc.org/1178703009/#ps240001 (title: "rebase and addressing comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1178703009/240001
Message was sent while issue was closed.
Committed patchset #10 (id:240001)
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/b69ab79338bff71ea411b82f3dd59508617a11d5 Cr-Commit-Position: refs/heads/master@{#9608} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
