Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2349843002:
Support more QCOM specific color formats for Android HW decoder. (Closed)
|
Created:
4 years, 3 months ago by AlexG Modified:
4 years, 3 months ago Reviewers:
magjed_webrtc CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, sakal Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionSupport more QCOM specific color formats for Android HW decoder.
BUG=b/31483393
Committed: https://crrev.com/893a7eeecbfcfc35177e35d4686312df177a62f9
Cr-Commit-Position: refs/heads/master@{#14359}
Patch Set 1 #
Total comments: 5
Patch Set 2 : Update comment #
Messages
Total messages: 12 (3 generated)
glaznev@webrtc.org changed reviewers: + magjed@webrtc.org
PTAL
https://codereview.webrtc.org/2349843002/diff/1/webrtc/api/android/jni/androi... File webrtc/api/android/jni/androidmediacodeccommon.h (right): https://codereview.webrtc.org/2349843002/diff/1/webrtc/api/android/jni/androi... webrtc/api/android/jni/androidmediacodeccommon.h:42: COLOR_QCOM_FORMATYVU420PackedSemiPlanar32m4ka = 0x7FA30C01, Don't you need to add these formats to the switch statement in androidmediaencoder_jni.cc? https://cs.chromium.org/chromium/src/third_party/webrtc/api/android/jni/andro...
https://codereview.webrtc.org/2349843002/diff/1/webrtc/api/android/jni/androi... File webrtc/api/android/jni/androidmediacodeccommon.h (right): https://codereview.webrtc.org/2349843002/diff/1/webrtc/api/android/jni/androi... webrtc/api/android/jni/androidmediacodeccommon.h:42: COLOR_QCOM_FORMATYVU420PackedSemiPlanar32m4ka = 0x7FA30C01, On 2016/09/17 08:52:30, magjed_webrtc wrote: > Don't you need to add these formats to the switch statement in > androidmediaencoder_jni.cc? > https://cs.chromium.org/chromium/src/third_party/webrtc/api/android/jni/andro... I don't think so. For decoder we request some specific color format, but then decoder is free to overwrite the format we requested using MediaCodec.INFO_OUTPUT_FORMAT_CHANGED. And we need to support if decoder decides to output some QCOM specific color space format. The encoder is different in terms is that we specify to the encoder what format we are going to send to, it can not treat it as different format and overwrite with its alignemnt specific format.
https://codereview.webrtc.org/2349843002/diff/1/webrtc/api/android/jni/androi... File webrtc/api/android/jni/androidmediacodeccommon.h (right): https://codereview.webrtc.org/2349843002/diff/1/webrtc/api/android/jni/androi... webrtc/api/android/jni/androidmediacodeccommon.h:42: COLOR_QCOM_FORMATYVU420PackedSemiPlanar32m4ka = 0x7FA30C01, On 2016/09/19 20:28:45, AlexG wrote: > On 2016/09/17 08:52:30, magjed_webrtc wrote: > > Don't you need to add these formats to the switch statement in > > androidmediaencoder_jni.cc? > > > https://cs.chromium.org/chromium/src/third_party/webrtc/api/android/jni/andro... > > I don't think so. For decoder we request some specific color format, but then > decoder is free to overwrite the format we requested using > MediaCodec.INFO_OUTPUT_FORMAT_CHANGED. And we need to support if decoder decides > to output some QCOM specific color space format. > > The encoder is different in terms is that we specify to the encoder what format > we are going to send to, it can not treat it as different format and overwrite > with its alignemnt specific format. Right. Is the plan to update the encoder in a follow-up CL? What I don't understand is why you have updated this enum without updating supportedColorList in MediaCodecVideoEncoder.java and the switch in androidmediaencoder_jni.cc. With your change, this enum does not mirror the encoder, contradicting the comment. The encoder is the only one actually using all values in this enum, the decoder is just using COLOR_FormatYUV420Planar.
https://codereview.webrtc.org/2349843002/diff/1/webrtc/api/android/jni/androi... File webrtc/api/android/jni/androidmediacodeccommon.h (right): https://codereview.webrtc.org/2349843002/diff/1/webrtc/api/android/jni/androi... webrtc/api/android/jni/androidmediacodeccommon.h:42: COLOR_QCOM_FORMATYVU420PackedSemiPlanar32m4ka = 0x7FA30C01, On 2016/09/20 09:12:41, magjed_webrtc wrote: > On 2016/09/19 20:28:45, AlexG wrote: > > On 2016/09/17 08:52:30, magjed_webrtc wrote: > > > Don't you need to add these formats to the switch statement in > > > androidmediaencoder_jni.cc? > > > > > > https://cs.chromium.org/chromium/src/third_party/webrtc/api/android/jni/andro... > > > > I don't think so. For decoder we request some specific color format, but then > > decoder is free to overwrite the format we requested using > > MediaCodec.INFO_OUTPUT_FORMAT_CHANGED. And we need to support if decoder > decides > > to output some QCOM specific color space format. > > > > The encoder is different in terms is that we specify to the encoder what > format > > we are going to send to, it can not treat it as different format and overwrite > > with its alignemnt specific format. > > Right. Is the plan to update the encoder in a follow-up CL? What I don't > understand is why you have updated this enum without updating supportedColorList > in MediaCodecVideoEncoder.java and the switch in androidmediaencoder_jni.cc. > With your change, this enum does not mirror the encoder, contradicting the > comment. The encoder is the only one actually using all values in this enum, the > decoder is just using COLOR_FormatYUV420Planar. No, I don't plan to update the encoder. Supported formats can be different for encoder and decoder. We support the case if decoder outputs QCOM format which uses some optimized alignment. But we do not support the case when we have to provide data to encoder in this format since we do not know exact alignment requirements. I updated the comment to reflect this
lgtm https://codereview.webrtc.org/2349843002/diff/1/webrtc/api/android/jni/androi... File webrtc/api/android/jni/androidmediacodeccommon.h (right): https://codereview.webrtc.org/2349843002/diff/1/webrtc/api/android/jni/androi... webrtc/api/android/jni/androidmediacodeccommon.h:42: COLOR_QCOM_FORMATYVU420PackedSemiPlanar32m4ka = 0x7FA30C01, On 2016/09/20 22:29:16, AlexG wrote: > On 2016/09/20 09:12:41, magjed_webrtc wrote: > > On 2016/09/19 20:28:45, AlexG wrote: > > > On 2016/09/17 08:52:30, magjed_webrtc wrote: > > > > Don't you need to add these formats to the switch statement in > > > > androidmediaencoder_jni.cc? > > > > > > > > > > https://cs.chromium.org/chromium/src/third_party/webrtc/api/android/jni/andro... > > > > > > I don't think so. For decoder we request some specific color format, but > then > > > decoder is free to overwrite the format we requested using > > > MediaCodec.INFO_OUTPUT_FORMAT_CHANGED. And we need to support if decoder > > decides > > > to output some QCOM specific color space format. > > > > > > The encoder is different in terms is that we specify to the encoder what > > format > > > we are going to send to, it can not treat it as different format and > overwrite > > > with its alignemnt specific format. > > > > Right. Is the plan to update the encoder in a follow-up CL? What I don't > > understand is why you have updated this enum without updating > supportedColorList > > in MediaCodecVideoEncoder.java and the switch in androidmediaencoder_jni.cc. > > With your change, this enum does not mirror the encoder, contradicting the > > comment. The encoder is the only one actually using all values in this enum, > the > > decoder is just using COLOR_FormatYUV420Planar. > > No, I don't plan to update the encoder. Supported formats can be different for > encoder and decoder. We support the case if decoder outputs QCOM format which > uses some optimized alignment. But we do not support the case when we have to > provide data to encoder in this format since we do not know exact alignment > requirements. > I updated the comment to reflect this Ok, sgtm.
The CQ bit was checked by glaznev@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Support more QCOM specific color formats for Android HW decoder. BUG=b/31483393 ========== to ========== Support more QCOM specific color formats for Android HW decoder. BUG=b/31483393 Committed: https://crrev.com/893a7eeecbfcfc35177e35d4686312df177a62f9 Cr-Commit-Position: refs/heads/master@{#14359} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/893a7eeecbfcfc35177e35d4686312df177a62f9 Cr-Commit-Position: refs/heads/master@{#14359} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
