Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2826453002:
Image Capture Android: only restore preview parameters after photo is taken (Closed)
|
Created:
3 years, 8 months ago by mcasas Modified:
3 years, 8 months ago Reviewers:
Reilly Grant (use Gerrit) CC:
chromium-reviews, posciak+watch_chromium.org, chfremer+watch_chromium.org, feature-media-reviews_chromium.org, agrieve+watch_chromium.org, xjz+watch_chromium.org, miu+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionImage Capture Android: only restore preview parameters after photo is taken
This CL introduces a member variable |mPreviewParameters| to
cache the preview capture parameters in order to restore them
after a picture is being taken and, more importantly (see bug),
delays restoring those parameters until after the picture is
actually taken.
While debugging these changes with a Nexus 7 and a Galaxy Note 2
7100, I reckoned that the Nexus 7 is very delicate so I added
some try-catches and avoided setting the picture resolution
if not truly set by the user (== use preview size).
BUG=706674
Review-Url: https://codereview.chromium.org/2826453002
Cr-Commit-Position: refs/heads/master@{#465118}
Committed: https://chromium.googlesource.com/chromium/src/+/a6c1b9d6d636b72d9b88cb2ab5723be4770d006f
Patch Set 1 : #
Total comments: 6
Patch Set 2 : reillyg@'s comments #Messages
Total messages: 17 (11 generated)
Description was changed from ========== Image Capture Android: delay restoring preview parameters to after photo is taken Moved restoring parameters to after taking the picture BUG=706674 ========== to ========== Image Capture Android: only restore preview parameters after photo is taken This CL introduces a member variable |mPreviewParameters| to cache the preview capture parameters in order to restore them after a picture is being taken and, more importantly (see bug), delays restoring those parameters until after the picture is actually taken. While debugging these changes with a Nexus 7 and a Galaxy Note 2 7100, I reckoned that the Nexus 7 is very delicate so I added some try-catches and avoided setting the picture resolution if not truly set by the user (== use preview size). BUG=706674 ==========
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
mcasas@chromium.org changed reviewers: + reillyg@chromium.org
reillyg@ PTAL
https://codereview.chromium.org/2826453002/diff/60001/media/capture/video/and... File media/capture/video/android/java/src/org/chromium/media/VideoCaptureCamera.java (right): https://codereview.chromium.org/2826453002/diff/60001/media/capture/video/and... media/capture/video/android/java/src/org/chromium/media/VideoCaptureCamera.java:93: private android.hardware.Camera.Parameters mPreviewParameters = null; In Java everything starts zero-initialized. https://codereview.chromium.org/2826453002/diff/60001/media/capture/video/and... media/capture/video/android/java/src/org/chromium/media/VideoCaptureCamera.java:175: } I think we should do these steps before resetting |mPhotoTakenCallbackId| or else they could conflict with another takePhoto() call. https://codereview.chromium.org/2826453002/diff/60001/media/capture/video/and... media/capture/video/android/java/src/org/chromium/media/VideoCaptureCamera.java:769: // |mPhotoWidth| and |mPhotoHeight| can only be set immediately before takePicture(), This comment is confusing because |mPhotoWidth| and |mPhotoHeight| are set earlier. The point is that we shouldn't call mCamera.setParameters() until immediately before takePicture().
ptal https://codereview.chromium.org/2826453002/diff/60001/media/capture/video/and... File media/capture/video/android/java/src/org/chromium/media/VideoCaptureCamera.java (right): https://codereview.chromium.org/2826453002/diff/60001/media/capture/video/and... media/capture/video/android/java/src/org/chromium/media/VideoCaptureCamera.java:93: private android.hardware.Camera.Parameters mPreviewParameters = null; On 2017/04/18 00:32:12, Reilly Grant wrote: > In Java everything starts zero-initialized. Oops too much C++ lately. Done. https://codereview.chromium.org/2826453002/diff/60001/media/capture/video/and... media/capture/video/android/java/src/org/chromium/media/VideoCaptureCamera.java:175: } On 2017/04/18 00:32:12, Reilly Grant wrote: > I think we should do these steps before resetting |mPhotoTakenCallbackId| or > else they could conflict with another takePhoto() call. Done. https://codereview.chromium.org/2826453002/diff/60001/media/capture/video/and... media/capture/video/android/java/src/org/chromium/media/VideoCaptureCamera.java:769: // |mPhotoWidth| and |mPhotoHeight| can only be set immediately before takePicture(), On 2017/04/18 00:32:12, Reilly Grant wrote: > This comment is confusing because |mPhotoWidth| and |mPhotoHeight| are set > earlier. The point is that we shouldn't call mCamera.setParameters() until > immediately before takePicture(). Yeah, it was a leftover of an intermediate step. As a matter of fact this comment doesn't add any relevant information so I'll remove it -- it's clear that in this method we're composing |photoParameters| to be passed to |mCamera|.
The CQ bit was checked by mcasas@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
The CQ bit was unchecked by mcasas@chromium.org
The CQ bit was checked by mcasas@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1492482334147920,
"parent_rev": "3db4686466461aa65dadcef4f4b96db53e425b9d", "commit_rev":
"a6c1b9d6d636b72d9b88cb2ab5723be4770d006f"}
Message was sent while issue was closed.
Description was changed from ========== Image Capture Android: only restore preview parameters after photo is taken This CL introduces a member variable |mPreviewParameters| to cache the preview capture parameters in order to restore them after a picture is being taken and, more importantly (see bug), delays restoring those parameters until after the picture is actually taken. While debugging these changes with a Nexus 7 and a Galaxy Note 2 7100, I reckoned that the Nexus 7 is very delicate so I added some try-catches and avoided setting the picture resolution if not truly set by the user (== use preview size). BUG=706674 ========== to ========== Image Capture Android: only restore preview parameters after photo is taken This CL introduces a member variable |mPreviewParameters| to cache the preview capture parameters in order to restore them after a picture is being taken and, more importantly (see bug), delays restoring those parameters until after the picture is actually taken. While debugging these changes with a Nexus 7 and a Galaxy Note 2 7100, I reckoned that the Nexus 7 is very delicate so I added some try-catches and avoided setting the picture resolution if not truly set by the user (== use preview size). BUG=706674 Review-Url: https://codereview.chromium.org/2826453002 Cr-Commit-Position: refs/heads/master@{#465118} Committed: https://chromium.googlesource.com/chromium/src/+/a6c1b9d6d636b72d9b88cb2ab572... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:80001) as https://chromium.googlesource.com/chromium/src/+/a6c1b9d6d636b72d9b88cb2ab572... |
