Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2511793002:
Remove Tango Chrome video capture support. (Closed)
|
Created:
4 years, 1 month ago by jam39 Modified:
4 years ago Reviewers:
mcasas CC:
chromium-reviews, posciak+watch_chromium.org, feature-media-reviews_chromium.org, mcasas+watch+vc_chromium.org, agrieve+watch_chromium.org, xjz+watch_chromium.org, miu+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove Tango Chrome video capture support.
https://bugs.chromium.org/p/chromium/issues/detail?id=664549
Also collapse VideoCaptureAndroid within VideoCaptureCamera.
BUG=664549
Committed: https://crrev.com/d5f7e33705dff425d0dce9e6377158f5f1feec1e
Cr-Commit-Position: refs/heads/master@{#435820}
Patch Set 1 #Patch Set 2 : Add missing imports. #Patch Set 3 : Attempt to fix build errors. #Patch Set 4 : Fix compiler issue again. #
Total comments: 15
Patch Set 5 : Changes after code review by mcasas. #
Total comments: 3
Patch Set 6 : Fixed build error;Code review changes #
Total comments: 4
Patch Set 7 : Fix import order. #
Messages
Total messages: 29 (16 generated)
Description was changed from ========== Remove Tango Chrome video capture support. https://bugs.chromium.org/p/chromium/issues/detail?id=664549 Also collapse VideoCaptureAndroid within VideoCaptureCamera. BUG=664549 ========== to ========== Remove Tango Chrome video capture support. https://bugs.chromium.org/p/chromium/issues/detail?id=664549 Also collapse VideoCaptureAndroid within VideoCaptureCamera. BUG=664549 ==========
jem456.vasishta@gmail.com changed reviewers: + mcasas@chromium.org
jem456.vasishta@gmail.com changed required reviewers: + mcasas@chromium.org
jem456.vasishta@gmail.com changed required reviewers: - mcasas@chromium.org
On 2016/11/18 18:01:29, jam wrote: > mailto:jem456.vasishta@gmail.com changed required reviewers: > - mailto:mcasas@chromium.org Looking good. Sort out the bots and, when ready, click on "Publish+mail comments" and write "PTAL", so I get a mail.
PTAL
PTAL
Getting there. https://codereview.chromium.org/2511793002/diff/60001/media/capture/video/and... File media/capture/video/android/java/src/org/chromium/media/VideoCaptureCamera.java (right): https://codereview.chromium.org/2511793002/diff/60001/media/capture/video/and... media/capture/video/android/java/src/org/chromium/media/VideoCaptureCamera.java:39: protected static final int GL_TEXTURE_EXTERNAL_OES = 0x8D65; This and other protected variables and methods don't need to be protected anymore, since there are no more derived classes, right? https://codereview.chromium.org/2511793002/diff/60001/media/capture/video/and... media/capture/video/android/java/src/org/chromium/media/VideoCaptureCamera.java:83: private static final int NUM_CAPTURE_BUFFERS = 3; nit: put this with the other private static members around l.38. https://codereview.chromium.org/2511793002/diff/60001/media/capture/video/and... media/capture/video/android/java/src/org/chromium/media/VideoCaptureCamera.java:694: static int getNumberOfCameras() { Group static methods above the constructor and below the private inner classes. https://codereview.chromium.org/2511793002/diff/60001/media/capture/video/and... media/capture/video/android/java/src/org/chromium/media/VideoCaptureCamera.java:784: private void allocateBuffers() { This method is only called once, so consider inlining it. https://codereview.chromium.org/2511793002/diff/60001/media/capture/video/and... media/capture/video/android/java/src/org/chromium/media/VideoCaptureCamera.java:793: private void setPreviewCallback(android.hardware.Camera.PreviewCallback cb) { @Override https://codereview.chromium.org/2511793002/diff/60001/media/capture/video/and... media/capture/video/android/java/src/org/chromium/media/VideoCaptureCamera.java:817: // performance and frame rate, using onFrameAvailable(). I'd remove these comments since they have no associated bug and they are ancient. https://codereview.chromium.org/2511793002/diff/60001/media/capture/video/and... File media/capture/video/android/java/src/org/chromium/media/VideoCaptureFactory.java (right): https://codereview.chromium.org/2511793002/diff/60001/media/capture/video/and... media/capture/video/android/java/src/org/chromium/media/VideoCaptureFactory.java:20: * ChromiumCameraInfo is an internal class with some static methods needed from nit: reflow the whole text to 80 chars?
PTAL https://codereview.chromium.org/2511793002/diff/60001/media/capture/video/and... File media/capture/video/android/java/src/org/chromium/media/VideoCaptureCamera.java (right): https://codereview.chromium.org/2511793002/diff/60001/media/capture/video/and... media/capture/video/android/java/src/org/chromium/media/VideoCaptureCamera.java:39: protected static final int GL_TEXTURE_EXTERNAL_OES = 0x8D65; On 2016/11/23 01:02:19, mcasas wrote: > This and other protected variables and methods don't need > to be protected anymore, since there are no more derived > classes, right? Done. https://codereview.chromium.org/2511793002/diff/60001/media/capture/video/and... media/capture/video/android/java/src/org/chromium/media/VideoCaptureCamera.java:83: private static final int NUM_CAPTURE_BUFFERS = 3; On 2016/11/23 01:02:19, mcasas wrote: > nit: put this with the other private static members around l.38. Done. https://codereview.chromium.org/2511793002/diff/60001/media/capture/video/and... media/capture/video/android/java/src/org/chromium/media/VideoCaptureCamera.java:694: static int getNumberOfCameras() { On 2016/11/23 01:02:19, mcasas wrote: > Group static methods above the constructor and below the private > inner classes. Done. https://codereview.chromium.org/2511793002/diff/60001/media/capture/video/and... media/capture/video/android/java/src/org/chromium/media/VideoCaptureCamera.java:784: private void allocateBuffers() { On 2016/11/23 01:02:19, mcasas wrote: > This method is only called once, so consider inlining it. Done. https://codereview.chromium.org/2511793002/diff/60001/media/capture/video/and... media/capture/video/android/java/src/org/chromium/media/VideoCaptureCamera.java:784: private void allocateBuffers() { On 2016/11/23 01:02:19, mcasas wrote: > This method is only called once, so consider inlining it. Should the same be done for setCaptureParameters as well? It seems to be called only once inside allocate(). In the next patchset, I've inlined setCaptureParameters as well. Please let me know if that was unnecessary. https://codereview.chromium.org/2511793002/diff/60001/media/capture/video/and... media/capture/video/android/java/src/org/chromium/media/VideoCaptureCamera.java:793: private void setPreviewCallback(android.hardware.Camera.PreviewCallback cb) { On 2016/11/23 01:02:19, mcasas wrote: > @Override Done. https://codereview.chromium.org/2511793002/diff/60001/media/capture/video/and... media/capture/video/android/java/src/org/chromium/media/VideoCaptureCamera.java:817: // performance and frame rate, using onFrameAvailable(). On 2016/11/23 01:02:19, mcasas wrote: > I'd remove these comments since they have no associated bug > and they are ancient. Done. https://codereview.chromium.org/2511793002/diff/60001/media/capture/video/and... File media/capture/video/android/java/src/org/chromium/media/VideoCaptureFactory.java (right): https://codereview.chromium.org/2511793002/diff/60001/media/capture/video/and... media/capture/video/android/java/src/org/chromium/media/VideoCaptureFactory.java:20: * ChromiumCameraInfo is an internal class with some static methods needed from On 2016/11/23 01:02:20, mcasas wrote: > nit: reflow the whole text to 80 chars? Done.
Last round of comments https://codereview.chromium.org/2511793002/diff/80001/media/capture/video/and... File media/capture/video/android/java/src/org/chromium/media/VideoCaptureCamera.java (right): https://codereview.chromium.org/2511793002/diff/80001/media/capture/video/and... media/capture/video/android/java/src/org/chromium/media/VideoCaptureCamera.java:352: new VideoCaptureFormat(width, height, frameRate, BuggyDeviceHack.getImageFormat()); this should be mCaptureFormat = new VideoCaptureFormat( matchedWidth, matchedHeight, chosenFrameRate, BuggyDeviceHack.getImageFormat()); https://codereview.chromium.org/2511793002/diff/80001/media/capture/video/and... File media/capture/video/android/java/src/org/chromium/media/VideoCaptureFactory.java (right): https://codereview.chromium.org/2511793002/diff/80001/media/capture/video/and... media/capture/video/android/java/src/org/chromium/media/VideoCaptureFactory.java:87: } else { No need for else {} after a return, follow the pattern in e.g. l. 97, 105.
PTAL https://codereview.chromium.org/2511793002/diff/80001/media/capture/video/and... File media/capture/video/android/java/src/org/chromium/media/VideoCaptureCamera.java (right): https://codereview.chromium.org/2511793002/diff/80001/media/capture/video/and... media/capture/video/android/java/src/org/chromium/media/VideoCaptureCamera.java:352: new VideoCaptureFormat(width, height, frameRate, BuggyDeviceHack.getImageFormat()); On 2016/11/28 17:47:52, mcasas wrote: > this should be > > mCaptureFormat = new VideoCaptureFormat( > matchedWidth, matchedHeight, chosenFrameRate, > BuggyDeviceHack.getImageFormat()); Copy-paste mistake, apologies for missing this one out!
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
lgtm % bots happy. https://codereview.chromium.org/2511793002/diff/100001/media/capture/video/an... File media/capture/video/android/java/src/org/chromium/media/VideoCaptureCamera.java (right): https://codereview.chromium.org/2511793002/diff/100001/media/capture/video/an... media/capture/video/android/java/src/org/chromium/media/VideoCaptureCamera.java:14: import android.graphics.ImageFormat; This line should go before l.9; (one of the bots said that too). https://codereview.chromium.org/2511793002/diff/100001/media/capture/video/an... media/capture/video/android/java/src/org/chromium/media/VideoCaptureCamera.java:21: import java.util.ArrayList; This line should go before l.20; (one of the bots said that too).
PTAL Thanks a lot for the review! Next, I'll now look for a bug where I have the proper hardware to debug with. :) https://codereview.chromium.org/2511793002/diff/100001/media/capture/video/an... File media/capture/video/android/java/src/org/chromium/media/VideoCaptureCamera.java (right): https://codereview.chromium.org/2511793002/diff/100001/media/capture/video/an... media/capture/video/android/java/src/org/chromium/media/VideoCaptureCamera.java:14: import android.graphics.ImageFormat; On 2016/11/30 21:30:30, mcasas wrote: > This line should go before l.9; > (one of the bots said that too). Done. https://codereview.chromium.org/2511793002/diff/100001/media/capture/video/an... media/capture/video/android/java/src/org/chromium/media/VideoCaptureCamera.java:21: import java.util.ArrayList; On 2016/11/30 21:30:30, mcasas wrote: > This line should go before l.20; > (one of the bots said that too). Done.
On 2016/12/01 17:49:48, jam wrote: > PTAL > > Thanks a lot for the review! > Next, I'll now look for a bug where I have the proper hardware to debug with. :) > > https://codereview.chromium.org/2511793002/diff/100001/media/capture/video/an... > File > media/capture/video/android/java/src/org/chromium/media/VideoCaptureCamera.java > (right): > > https://codereview.chromium.org/2511793002/diff/100001/media/capture/video/an... > media/capture/video/android/java/src/org/chromium/media/VideoCaptureCamera.java:14: > import android.graphics.ImageFormat; > On 2016/11/30 21:30:30, mcasas wrote: > > This line should go before l.9; > > (one of the bots said that too). > > Done. > > https://codereview.chromium.org/2511793002/diff/100001/media/capture/video/an... > media/capture/video/android/java/src/org/chromium/media/VideoCaptureCamera.java:21: > import java.util.ArrayList; > On 2016/11/30 21:30:30, mcasas wrote: > > This line should go before l.20; > > (one of the bots said that too). > > Done. still LGTM. A Nexus 7.2 should use the Camera2 legacy API and fall back to VideoCaptureCamera.java.
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by jem456.vasishta@gmail.com
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": 120001, "attempt_start_ts": 1480643149134110,
"parent_rev": "f03dce2da3642403e4ca14c0bf5bae0500ace2d6", "commit_rev":
"cba9124348fa282716c4736754e5fd586eaad371"}
Message was sent while issue was closed.
Description was changed from ========== Remove Tango Chrome video capture support. https://bugs.chromium.org/p/chromium/issues/detail?id=664549 Also collapse VideoCaptureAndroid within VideoCaptureCamera. BUG=664549 ========== to ========== Remove Tango Chrome video capture support. https://bugs.chromium.org/p/chromium/issues/detail?id=664549 Also collapse VideoCaptureAndroid within VideoCaptureCamera. BUG=664549 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Remove Tango Chrome video capture support. https://bugs.chromium.org/p/chromium/issues/detail?id=664549 Also collapse VideoCaptureAndroid within VideoCaptureCamera. BUG=664549 ========== to ========== Remove Tango Chrome video capture support. https://bugs.chromium.org/p/chromium/issues/detail?id=664549 Also collapse VideoCaptureAndroid within VideoCaptureCamera. BUG=664549 Committed: https://crrev.com/d5f7e33705dff425d0dce9e6377158f5f1feec1e Cr-Commit-Position: refs/heads/master@{#435820} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/d5f7e33705dff425d0dce9e6377158f5f1feec1e Cr-Commit-Position: refs/heads/master@{#435820} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
