Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2004973003:
Enable VP8 and H.264 HW encoders on Exynos devices. (Closed)
DescriptionEnable VP8 and H.264 HW encoders on Exynos devices.
Run VP8 HW only on M and above devices.
Exynos H.264 HW encoder does not use frame timestamps for
bitrate allocation, so bitrate target need to be adjusted based
on current camera frame rate.
BUG=b/28738766
R=magjed@webrtc.org
Committed: https://crrev.com/269fe75a962fb26a0686ef8c8cd101fb71903d6f
Cr-Commit-Position: refs/heads/master@{#12900}
Patch Set 1 #
Total comments: 14
Patch Set 2 : Address comments #
Total comments: 2
Patch Set 3 : Address comments 2 #
Messages
Total messages: 15 (5 generated)
glaznev@webrtc.org changed reviewers: + magjed@webrtc.org
PTAL
https://codereview.webrtc.org/2004973003/diff/1/webrtc/api/java/src/org/webrt... File webrtc/api/java/src/org/webrtc/MediaCodecVideoEncoder.java (right): https://codereview.webrtc.org/2004973003/diff/1/webrtc/api/java/src/org/webrt... webrtc/api/java/src/org/webrtc/MediaCodecVideoEncoder.java:86: // Class describing known media codec restriciotns. nit: restrictions https://codereview.webrtc.org/2004973003/diff/1/webrtc/api/java/src/org/webrt... webrtc/api/java/src/org/webrtc/MediaCodecVideoEncoder.java:95: public final boolean timestapmsAreNotUsed; nit: timestapms is misspelled. Also, I would prefer a positive connotation, i.e. timestampsAreUsed instead of negated name. https://codereview.webrtc.org/2004973003/diff/1/webrtc/api/java/src/org/webrt... webrtc/api/java/src/org/webrtc/MediaCodecVideoEncoder.java:108: "OMX.Exynos.", VP8_MIME_TYPE, Build.VERSION_CODES.M, false); nit: Comment literal argument: false /* timestampsAreNotUsed */ (or rather: true /* timestampsAreUsed */). https://codereview.webrtc.org/2004973003/diff/1/webrtc/api/java/src/org/webrt... webrtc/api/java/src/org/webrtc/MediaCodecVideoEncoder.java:288: return null; Should we really abort here and not check more color formats? https://codereview.webrtc.org/2004973003/diff/1/webrtc/api/java/src/org/webrt... webrtc/api/java/src/org/webrtc/MediaCodecVideoEncoder.java:291: Logging.w(TAG, "Codec does not use frme timestamps."); nit: frme is misspelled https://codereview.webrtc.org/2004973003/diff/1/webrtc/api/java/src/org/webrt... webrtc/api/java/src/org/webrtc/MediaCodecVideoEncoder.java:294: break; Replace 'break' with 'return new EncoderProperties(name, codecColorFormat, !mediaCodecRestriction.timestampsAreUsed)' instead? https://codereview.webrtc.org/2004973003/diff/1/webrtc/api/java/src/org/webrt... webrtc/api/java/src/org/webrtc/MediaCodecVideoEncoder.java:524: codecBitrate = (int)(BITRATE_ADJUSTMENT_FPS * codecBitrate / (float)frameRate); If you are casting to int anyway, you can remove float cast and use 'BITRATE_ADJUSTMENT_FPS * codecBitrate / frameRate' directly.
https://codereview.webrtc.org/2004973003/diff/1/webrtc/api/java/src/org/webrt... File webrtc/api/java/src/org/webrtc/MediaCodecVideoEncoder.java (right): https://codereview.webrtc.org/2004973003/diff/1/webrtc/api/java/src/org/webrt... webrtc/api/java/src/org/webrtc/MediaCodecVideoEncoder.java:86: // Class describing known media codec restriciotns. On 2016/05/24 08:53:09, magjed_webrtc wrote: > nit: restrictions Done. https://codereview.webrtc.org/2004973003/diff/1/webrtc/api/java/src/org/webrt... webrtc/api/java/src/org/webrtc/MediaCodecVideoEncoder.java:95: public final boolean timestapmsAreNotUsed; On 2016/05/24 08:53:09, magjed_webrtc wrote: > nit: timestapms is misspelled. Also, I would prefer a positive connotation, i.e. > timestampsAreUsed instead of negated name. Done. Changed to positive connotations. But it is ptobably a little safer for false value meaning no changes in bitrate adjustment are required, so I moved to "bitrateAdjustmentRequired" name. https://codereview.webrtc.org/2004973003/diff/1/webrtc/api/java/src/org/webrt... webrtc/api/java/src/org/webrtc/MediaCodecVideoEncoder.java:108: "OMX.Exynos.", VP8_MIME_TYPE, Build.VERSION_CODES.M, false); On 2016/05/24 08:53:09, magjed_webrtc wrote: > nit: Comment literal argument: false /* timestampsAreNotUsed */ (or rather: true > /* timestampsAreUsed */). Done. https://codereview.webrtc.org/2004973003/diff/1/webrtc/api/java/src/org/webrt... webrtc/api/java/src/org/webrtc/MediaCodecVideoEncoder.java:288: return null; On 2016/05/24 08:53:09, magjed_webrtc wrote: > Should we really abort here and not check more color formats? Yes, I think we should abort here - restrictions is applied for all color formats. I moved the restriction check out of color check loop. https://codereview.webrtc.org/2004973003/diff/1/webrtc/api/java/src/org/webrt... webrtc/api/java/src/org/webrtc/MediaCodecVideoEncoder.java:291: Logging.w(TAG, "Codec does not use frme timestamps."); On 2016/05/24 08:53:09, magjed_webrtc wrote: > nit: frme is misspelled Done. https://codereview.webrtc.org/2004973003/diff/1/webrtc/api/java/src/org/webrt... webrtc/api/java/src/org/webrtc/MediaCodecVideoEncoder.java:294: break; On 2016/05/24 08:53:09, magjed_webrtc wrote: > Replace 'break' with 'return new EncoderProperties(name, codecColorFormat, > !mediaCodecRestriction.timestampsAreUsed)' instead? Done. Moved restriction check out of color check loop. https://codereview.webrtc.org/2004973003/diff/1/webrtc/api/java/src/org/webrt... webrtc/api/java/src/org/webrtc/MediaCodecVideoEncoder.java:524: codecBitrate = (int)(BITRATE_ADJUSTMENT_FPS * codecBitrate / (float)frameRate); On 2016/05/24 08:53:09, magjed_webrtc wrote: > If you are casting to int anyway, you can remove float cast and use > 'BITRATE_ADJUSTMENT_FPS * codecBitrate / frameRate' directly. Done.
https://codereview.webrtc.org/2004973003/diff/20001/webrtc/api/java/src/org/w... File webrtc/api/java/src/org/webrtc/MediaCodecVideoEncoder.java (right): https://codereview.webrtc.org/2004973003/diff/20001/webrtc/api/java/src/org/w... webrtc/api/java/src/org/webrtc/MediaCodecVideoEncoder.java:83: private static final String[] supportedH264HwCodecPrefixes = These arrays of supported codecs are very overlapping with the new MediaCodecRestriction. Can you replace supported{Vp8/Vp9/H264}HwCodecPrefixes and hwCodecRestrictions with one array 'MediaCodecRestriction[] supportedCodecs' instead? That would also simplify the code in findHwEncoder() and remove the 'String[] supportedHwCodecPrefixes' argument to that function.
pbos@webrtc.org changed reviewers: + pbos@webrtc.org, tkchin@webrtc.org
Can you sync with tkchin@ to see if his BitrateAdjuster can be used for the same purpose instead of dividing by frame rates when they arrive? This should also prevent a regression if the Exynos drivers are ever fixed.
On 2016/05/25 12:06:01, pbos-webrtc wrote: > Can you sync with tkchin@ to see if his BitrateAdjuster can be used for the same > purpose instead of dividing by frame rates when they arrive? This should also > prevent a regression if the Exynos drivers are ever fixed. This adjuster addresses specific problem, so it is better than Zeke's general purpose adjuster to fix this particular codec bug since it reacts faster - as soon as frame rate is changed and generates accurate bitrate correction immediately. We may need to come with general purpose solution later, but this is out of scope for this CL
PTAL https://codereview.webrtc.org/2004973003/diff/20001/webrtc/api/java/src/org/w... File webrtc/api/java/src/org/webrtc/MediaCodecVideoEncoder.java (right): https://codereview.webrtc.org/2004973003/diff/20001/webrtc/api/java/src/org/w... webrtc/api/java/src/org/webrtc/MediaCodecVideoEncoder.java:83: private static final String[] supportedH264HwCodecPrefixes = On 2016/05/25 10:57:39, magjed_webrtc wrote: > These arrays of supported codecs are very overlapping with the new > MediaCodecRestriction. Can you replace supported{Vp8/Vp9/H264}HwCodecPrefixes > and hwCodecRestrictions with one array 'MediaCodecRestriction[] supportedCodecs' > instead? That would also simplify the code in findHwEncoder() and remove the > 'String[] supportedHwCodecPrefixes' argument to that function. Done.
lgtm
The CQ bit was checked by glaznev@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2004973003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2004973003/40001
Description was changed from ========== Enable VP8 and H.264 HW encoders on Exynos devices. Run VP8 HW only on M and above devices. Exynos H.264 HW encoder does not use frame timestamps for bitrate allocation, so bitrate target need to be adjusted based on current camera frame rate. BUG=b/28738766 ========== to ========== Enable VP8 and H.264 HW encoders on Exynos devices. Run VP8 HW only on M and above devices. Exynos H.264 HW encoder does not use frame timestamps for bitrate allocation, so bitrate target need to be adjusted based on current camera frame rate. BUG=b/28738766 R=magjed@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/269fe75a962fb26a0686ef8c8... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) manually as 269fe75a962fb26a0686ef8c8cd101fb71903d6f (presubmit successful).
Message was sent while issue was closed.
Description was changed from ========== Enable VP8 and H.264 HW encoders on Exynos devices. Run VP8 HW only on M and above devices. Exynos H.264 HW encoder does not use frame timestamps for bitrate allocation, so bitrate target need to be adjusted based on current camera frame rate. BUG=b/28738766 R=magjed@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/269fe75a962fb26a0686ef8c8... ========== to ========== Enable VP8 and H.264 HW encoders on Exynos devices. Run VP8 HW only on M and above devices. Exynos H.264 HW encoder does not use frame timestamps for bitrate allocation, so bitrate target need to be adjusted based on current camera frame rate. BUG=b/28738766 R=magjed@webrtc.org Committed: https://crrev.com/269fe75a962fb26a0686ef8c8cd101fb71903d6f Cr-Commit-Position: refs/heads/master@{#12900} ========== |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
