Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2722803002:
Android HW decoder: Limit frame size based on cropping information (Closed)
|
Created:
3 years, 9 months ago by magjed_webrtc Modified:
3 years, 9 months ago Reviewers:
sakal CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAndroid HW decoder: Limit frame size based on cropping information
BUG=webrtc:7267
Review-Url: https://codereview.webrtc.org/2722803002
Cr-Commit-Position: refs/heads/master@{#16963}
Committed: https://chromium.googlesource.com/external/webrtc/+/0c29de5579c9a07f9aa2f91e1d48797441c51189
Patch Set 1 #
Total comments: 8
Patch Set 2 : Address comments. #Messages
Total messages: 19 (13 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/v2/patch-status/codereview.webrtc.org/...
Description was changed from ========== Android HW decoder: Limit frame size based on cropping information BUG=webrtc:7267 ========== to ========== Android HW decoder: Limit frame size based on cropping information BUG=webrtc:7267 ==========
magjed@webrtc.org changed reviewers: + sakal@webrtc.org
Sami - please take a look.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.webrtc.org/2722803002/diff/1/webrtc/sdk/android/api/org/we... File webrtc/sdk/android/api/org/webrtc/MediaCodecVideoDecoder.java (right): https://codereview.webrtc.org/2722803002/diff/1/webrtc/sdk/android/api/org/we... webrtc/sdk/android/api/org/webrtc/MediaCodecVideoDecoder.java:44: private static final String FORMAT_KEY_STRIDE = "stride"; nit: Can you add a TODO to replace these with constants from MediaFormat once they are part of an API level we support. https://codereview.webrtc.org/2722803002/diff/1/webrtc/sdk/android/api/org/we... webrtc/sdk/android/api/org/webrtc/MediaCodecVideoDecoder.java:606: int new_width = format.getInteger(MediaFormat.KEY_WIDTH); nit: Can we change these to be camel case to follow the style guide? https://codereview.webrtc.org/2722803002/diff/1/webrtc/sdk/android/api/org/we... webrtc/sdk/android/api/org/webrtc/MediaCodecVideoDecoder.java:608: if (format.containsKey(FORMAT_KEY_CROP_LEFT) I would prefer: final int newWidth; final int newHeight; if (formatHasCropValues(format)) { ... calculate from crop values ... } else { ... get width & height from format ... } https://codereview.webrtc.org/2722803002/diff/1/webrtc/sdk/android/api/org/we... webrtc/sdk/android/api/org/webrtc/MediaCodecVideoDecoder.java:612: new_width = Math.min(new_width, crop_width); Are these min statements needed? Chromium code doesn't have them. https://cs.chromium.org/chromium/src/media/base/android/java/src/org/chromium...
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/v2/patch-status/codereview.webrtc.org/...
https://codereview.webrtc.org/2722803002/diff/1/webrtc/sdk/android/api/org/we... File webrtc/sdk/android/api/org/webrtc/MediaCodecVideoDecoder.java (right): https://codereview.webrtc.org/2722803002/diff/1/webrtc/sdk/android/api/org/we... webrtc/sdk/android/api/org/webrtc/MediaCodecVideoDecoder.java:44: private static final String FORMAT_KEY_STRIDE = "stride"; On 2017/03/01 12:49:14, sakal wrote: > nit: Can you add a TODO to replace these with constants from MediaFormat once > they are part of an API level we support. Done. https://codereview.webrtc.org/2722803002/diff/1/webrtc/sdk/android/api/org/we... webrtc/sdk/android/api/org/webrtc/MediaCodecVideoDecoder.java:606: int new_width = format.getInteger(MediaFormat.KEY_WIDTH); On 2017/03/01 12:49:14, sakal wrote: > nit: Can we change these to be camel case to follow the style guide? Done. https://codereview.webrtc.org/2722803002/diff/1/webrtc/sdk/android/api/org/we... webrtc/sdk/android/api/org/webrtc/MediaCodecVideoDecoder.java:608: if (format.containsKey(FORMAT_KEY_CROP_LEFT) On 2017/03/01 12:49:14, sakal wrote: > I would prefer: > final int newWidth; > final int newHeight; > if (formatHasCropValues(format)) { > ... calculate from crop values ... > } else { > ... get width & height from format ... > } Done. https://codereview.webrtc.org/2722803002/diff/1/webrtc/sdk/android/api/org/we... webrtc/sdk/android/api/org/webrtc/MediaCodecVideoDecoder.java:612: new_width = Math.min(new_width, crop_width); On 2017/03/01 12:49:15, sakal wrote: > Are these min statements needed? Chromium code doesn't have them. > https://cs.chromium.org/chromium/src/media/base/android/java/src/org/chromium... I wanted to be conservative, but if Chromium doesn't need them, we don't either.
lgtm
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 magjed@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1488444810963770,
"parent_rev": "55eb6d621489730084927868fed195d3645a9ec9", "commit_rev":
"0c29de5579c9a07f9aa2f91e1d48797441c51189"}
Message was sent while issue was closed.
Description was changed from ========== Android HW decoder: Limit frame size based on cropping information BUG=webrtc:7267 ========== to ========== Android HW decoder: Limit frame size based on cropping information BUG=webrtc:7267 Review-Url: https://codereview.webrtc.org/2722803002 Cr-Commit-Position: refs/heads/master@{#16963} Committed: https://chromium.googlesource.com/external/webrtc/+/0c29de5579c9a07f9aa2f91e1... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/external/webrtc/+/0c29de5579c9a07f9aa2f91e1... |
