Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
605883002:
Fix wrong selection of VP8 video decoder on some Samsung devices. (Closed)
|
Created:
6 years, 2 months ago by AlexGlaznev Modified:
6 years, 2 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org, avayvod+watch_chromium.org, tnakamura Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionFix wrong selection of VP8 video decoder on some Samsung devices.
- Create HW decoder using first codec in MediaCoedcList instead of
using createDecoderByType function. Later may result in opening
OMX.SEC.vp8.dec decoder, which can not be used for surface decoding.
- Revert r279297 and mark all OMX.SEC.* codecs as non HW accelerated.
BUG=408353, 396578
R=qinmin@chromium.org
Committed: https://crrev.com/6e86e4f1154c5f26ea343b635d3e218a3495da7e
Cr-Commit-Position: refs/heads/master@{#297667}
Patch Set 1 #
Total comments: 5
Patch Set 2 : Changing comments. #Patch Set 3 : Change IsKnownUnaccelerated to use default codec name instead of first codec in MediaCodecList. #
Total comments: 1
Patch Set 4 : Addressing nit #
Messages
Total messages: 15 (2 generated)
glaznev@chromium.org changed reviewers: + dalecurtis@chromium.org
PTAL
https://codereview.chromium.org/605883002/diff/1/media/base/android/java/src/... File media/base/android/java/src/org/chromium/media/MediaCodecBridge.java (right): https://codereview.chromium.org/605883002/diff/1/media/base/android/java/src/... media/base/android/java/src/org/chromium/media/MediaCodecBridge.java:242: mediaCodec = MediaCodec.createByCodecName(decoderName); Are you sure this will not cause any other issues? AFAIK, Calling createDecoderByType() will automatically switch to software decoder if hardware decoder is currently being used. However, if we call createByCodecName() twice on the hardware decoder, the 2nd call will fail. https://codereview.chromium.org/605883002/diff/1/media/base/android/media_cod... File media/base/android/media_codec_bridge.cc (right): https://codereview.chromium.org/605883002/diff/1/media/base/android/media_cod... media/base/android/media_codec_bridge.cc:180: // surface, so report it as SW decoder as well. the comments is wierd. This function is checking whether the decoder is HW accelerated or not, it has nothing to do with whether it can decode to a surface. The only place that we call this function is android_video_decode_accelerator.cc and android_video_encode_accelerator.cc
https://codereview.chromium.org/605883002/diff/1/media/base/android/java/src/... File media/base/android/java/src/org/chromium/media/MediaCodecBridge.java (right): https://codereview.chromium.org/605883002/diff/1/media/base/android/java/src/... media/base/android/java/src/org/chromium/media/MediaCodecBridge.java:242: mediaCodec = MediaCodec.createByCodecName(decoderName); On 2014/09/26 05:00:28, qinmin wrote: > Are you sure this will not cause any other issues? No, I am not 100% sure. But I know, that current way of codec selection is causing issues on majority of Samsung devices. > AFAIK, Calling createDecoderByType() will automatically switch to software > decoder if hardware decoder is currently being used. However, if we call > createByCodecName() twice on the hardware decoder, the 2nd call will fail. Yes, you are right - probably not on a second call, but after all HW codec instances will be used - probably after 7 - 12 calls. Current Android GTS tests require at least 3 encoder and 5 decoder instances to run in parallel (all instances are created using createByCodecName). But the drawback of using createDecoderByType is that it can pick up non HW accelerated or buggy codec (and this is what is happening for Samsung devices) or fall back to this bad codec after all available instances of HW codec will be used. So I think there could be two possible solutions: 1. Continue to use CreateEncoder{Decoder}ByType, but change MediaCodecBridge::IsKnownUnaccelerated to obtain the name of the default codec (it will require opening codec instance with CreateEncoder{Decoder}ByType and calling MediaCodec.getName()) instead of the top codec in MediaCodecList. Then android_video_decoder_accelerator.cc will not try to create and use MediaCodec for devices with default OMX.google or OMX.SEC codecs. 2. Approach in this CL - for video decoder use CreateByCodecName and expect that it will fail after all HW codec instances will be used. Please advise what do you think will be a better choice. https://codereview.chromium.org/605883002/diff/1/media/base/android/media_cod... File media/base/android/media_codec_bridge.cc (right): https://codereview.chromium.org/605883002/diff/1/media/base/android/media_cod... media/base/android/media_codec_bridge.cc:180: // surface, so report it as SW decoder as well. On 2014/09/26 05:00:28, qinmin wrote: > the comments is wierd. This function is checking whether the decoder is HW > accelerated or not, it has nothing to do with whether it can decode to a > surface. > The only place that we call this function is android_video_decode_accelerator.cc > and android_video_encode_accelerator.cc Done.
https://codereview.chromium.org/605883002/diff/1/media/base/android/java/src/... File media/base/android/java/src/org/chromium/media/MediaCodecBridge.java (right): https://codereview.chromium.org/605883002/diff/1/media/base/android/java/src/... media/base/android/java/src/org/chromium/media/MediaCodecBridge.java:242: mediaCodec = MediaCodec.createByCodecName(decoderName); Can you explain why CreateDecoderByType() will end up with buggy codecs? I checked findMatchingCodecs() in OMXCodec.cpp, that's how CreateDecoderByType() is implemented in native site. I saw that it also uses MediaCodecList to retrieve a list of codecs, and return the list to ACodec, which normally picks the first one. This implementation is pretty much the same as getDecoderNameForMime() here. So I don't know why changing from CreateDecoderByType() to createByCodecName() solves the issue. On 2014/09/26 18:01:37, AlexGlaznev wrote: > On 2014/09/26 05:00:28, qinmin wrote: > > Are you sure this will not cause any other issues? > > No, I am not 100% sure. But I know, that current way of codec selection is > causing issues on majority of Samsung devices. > > > AFAIK, Calling createDecoderByType() will automatically switch to software > > decoder if hardware decoder is currently being used. However, if we call > > createByCodecName() twice on the hardware decoder, the 2nd call will fail. > > Yes, you are right - probably not on a second call, but after all HW codec > instances will be used - probably after 7 - 12 calls. Current Android GTS tests > require at least 3 encoder and 5 decoder instances to run in parallel (all > instances are created using createByCodecName). > > But the drawback of using createDecoderByType is that it can pick up non HW > accelerated or buggy codec (and this is what is happening for Samsung devices) > or fall back to this bad codec after all available instances of HW codec will be > used. > > So I think there could be two possible solutions: > > 1. Continue to use CreateEncoder{Decoder}ByType, but change > MediaCodecBridge::IsKnownUnaccelerated to obtain the name of the default codec > (it will require opening codec instance with CreateEncoder{Decoder}ByType and > calling MediaCodec.getName()) instead of the top codec in MediaCodecList. Then > android_video_decoder_accelerator.cc will not try to create and use MediaCodec > for devices with default OMX.google or OMX.SEC codecs. > > 2. Approach in this CL - for video decoder use CreateByCodecName and expect that > it will fail after all HW codec instances will be used. > > Please advise what do you think will be a better choice. > >
On 2014/09/26 22:45:55, qinmin wrote: > https://codereview.chromium.org/605883002/diff/1/media/base/android/java/src/... > File media/base/android/java/src/org/chromium/media/MediaCodecBridge.java > (right): > > https://codereview.chromium.org/605883002/diff/1/media/base/android/java/src/... > media/base/android/java/src/org/chromium/media/MediaCodecBridge.java:242: > mediaCodec = MediaCodec.createByCodecName(decoderName); > Can you explain why CreateDecoderByType() will end up with buggy codecs? > I checked findMatchingCodecs() in OMXCodec.cpp, that's how CreateDecoderByType() > is implemented in native site. I saw that it also uses MediaCodecList to > retrieve a list of codecs, and return the list to ACodec, which normally picks > the first one. > This implementation is pretty much the same as getDecoderNameForMime() here. So > I don't know why changing from CreateDecoderByType() to createByCodecName() > solves the issue. > > On 2014/09/26 18:01:37, AlexGlaznev wrote: > > On 2014/09/26 05:00:28, qinmin wrote: > > > Are you sure this will not cause any other issues? > > > > No, I am not 100% sure. But I know, that current way of codec selection is > > causing issues on majority of Samsung devices. > > > > > AFAIK, Calling createDecoderByType() will automatically switch to software > > > decoder if hardware decoder is currently being used. However, if we call > > > createByCodecName() twice on the hardware decoder, the 2nd call will fail. > > > > Yes, you are right - probably not on a second call, but after all HW codec > > instances will be used - probably after 7 - 12 calls. Current Android GTS > tests > > require at least 3 encoder and 5 decoder instances to run in parallel (all > > instances are created using createByCodecName). > > > > But the drawback of using createDecoderByType is that it can pick up non HW > > accelerated or buggy codec (and this is what is happening for Samsung devices) > > or fall back to this bad codec after all available instances of HW codec will > be > > used. > > > > So I think there could be two possible solutions: > > > > 1. Continue to use CreateEncoder{Decoder}ByType, but change > > MediaCodecBridge::IsKnownUnaccelerated to obtain the name of the default codec > > (it will require opening codec instance with CreateEncoder{Decoder}ByType and > > calling MediaCodec.getName()) instead of the top codec in MediaCodecList. Then > > android_video_decoder_accelerator.cc will not try to create and use MediaCodec > > for devices with default OMX.google or OMX.SEC codecs. > > > > 2. Approach in this CL - for video decoder use CreateByCodecName and expect > that > > it will fail after all HW codec instances will be used. > > > > Please advise what do you think will be a better choice. > > > > I am not sure, but I think the requirement that MediaCodecList contains the codec sorted in the order of preference will appear for L only (comment #25 to b/9735008). For KK devices the default codec (opened by createDecoderByType) may not be the first one in MediaCodecList. For example for Galaxy S4 I9505 MediaCodecList contains following VP8 codecs: OMX.qcom.video.decoder.vp8, OMX.SEC.vp8.dec, OMX.google.vpx.decode so MediaCodecBridge.getCodecsInfo, which iterates through this list will create CodecInfo with "OMX.qcom.video.decoder.vp8" for "video/x-vnd.on2.vp8" and IsKnownUnaccelerated will return false. However when later MediaCodec is created using CreateDecoderByType "OMX.SEC.vp8.dec" is actually created, which can not be used for surface decoder (it will either fail or generate black output). However so far this is the only device I know which has this problem - for other Samsung devices default video codec and top codec in MediaCodecList are the same. So if not that S4 I9505 changing MediaCodecBridge::IsKnownUnaccelerated to report OMX.SEC. codec as unaccelerated will be enough. Probably to minimize the risk I can leave MediaCodecBridge.java untouched and only do this minimal change to MediaCodecBridge::IsKnownUnaccelerated and leave I9505 in video decoder black list.
On 2014/09/26 23:19:07, AlexGlaznev wrote: > On 2014/09/26 22:45:55, qinmin wrote: > > > https://codereview.chromium.org/605883002/diff/1/media/base/android/java/src/... > > File media/base/android/java/src/org/chromium/media/MediaCodecBridge.java > > (right): > > > > > https://codereview.chromium.org/605883002/diff/1/media/base/android/java/src/... > > media/base/android/java/src/org/chromium/media/MediaCodecBridge.java:242: > > mediaCodec = MediaCodec.createByCodecName(decoderName); > > Can you explain why CreateDecoderByType() will end up with buggy codecs? > > I checked findMatchingCodecs() in OMXCodec.cpp, that's how > CreateDecoderByType() > > is implemented in native site. I saw that it also uses MediaCodecList to > > retrieve a list of codecs, and return the list to ACodec, which normally picks > > the first one. > > This implementation is pretty much the same as getDecoderNameForMime() here. > So > > I don't know why changing from CreateDecoderByType() to createByCodecName() > > solves the issue. > > > > On 2014/09/26 18:01:37, AlexGlaznev wrote: > > > On 2014/09/26 05:00:28, qinmin wrote: > > > > Are you sure this will not cause any other issues? > > > > > > No, I am not 100% sure. But I know, that current way of codec selection is > > > causing issues on majority of Samsung devices. > > > > > > > AFAIK, Calling createDecoderByType() will automatically switch to software > > > > decoder if hardware decoder is currently being used. However, if we call > > > > createByCodecName() twice on the hardware decoder, the 2nd call will fail. > > > > > > Yes, you are right - probably not on a second call, but after all HW codec > > > instances will be used - probably after 7 - 12 calls. Current Android GTS > > tests > > > require at least 3 encoder and 5 decoder instances to run in parallel (all > > > instances are created using createByCodecName). > > > > > > But the drawback of using createDecoderByType is that it can pick up non HW > > > accelerated or buggy codec (and this is what is happening for Samsung > devices) > > > or fall back to this bad codec after all available instances of HW codec > will > > be > > > used. > > > > > > So I think there could be two possible solutions: > > > > > > 1. Continue to use CreateEncoder{Decoder}ByType, but change > > > MediaCodecBridge::IsKnownUnaccelerated to obtain the name of the default > codec > > > (it will require opening codec instance with CreateEncoder{Decoder}ByType > and > > > calling MediaCodec.getName()) instead of the top codec in MediaCodecList. > Then > > > android_video_decoder_accelerator.cc will not try to create and use > MediaCodec > > > for devices with default OMX.google or OMX.SEC codecs. > > > > > > 2. Approach in this CL - for video decoder use CreateByCodecName and expect > > that > > > it will fail after all HW codec instances will be used. > > > > > > Please advise what do you think will be a better choice. > > > > > > > > I am not sure, but I think the requirement that MediaCodecList contains the > codec sorted in the order of preference will appear for L only (comment #25 to > b/9735008). > For KK devices the default codec (opened by createDecoderByType) may not be the > first one in MediaCodecList. > > For example for Galaxy S4 I9505 MediaCodecList contains following VP8 codecs: > OMX.qcom.video.decoder.vp8, > OMX.SEC.vp8.dec, > OMX.google.vpx.decode > so MediaCodecBridge.getCodecsInfo, which iterates through this list will create > CodecInfo with "OMX.qcom.video.decoder.vp8" for "video/x-vnd.on2.vp8" and > IsKnownUnaccelerated will return false. > However when later MediaCodec is created using CreateDecoderByType > "OMX.SEC.vp8.dec" is actually created, which can not be used for surface decoder > (it will either fail or generate black output). > > However so far this is the only device I know which has this problem - for other > Samsung devices default video codec and top codec in MediaCodecList are the > same. So if not that S4 I9505 changing MediaCodecBridge::IsKnownUnaccelerated to > report OMX.SEC. codec as unaccelerated will be enough. > > Probably to minimize the risk I can leave MediaCodecBridge.java untouched and > only do this minimal change to MediaCodecBridge::IsKnownUnaccelerated and leave > I9505 in video decoder black list. ok, blacklist I9505 sounds good to me. If L will maintain codecs in sorted order, then we should try using createDecoderByType(). Or otherwise, we will have another if-else statement for different android versions that we don't know whether it works.
On 2014/09/26 23:34:19, qinmin wrote: > On 2014/09/26 23:19:07, AlexGlaznev wrote: > > On 2014/09/26 22:45:55, qinmin wrote: > > > > > > https://codereview.chromium.org/605883002/diff/1/media/base/android/java/src/... > > > File media/base/android/java/src/org/chromium/media/MediaCodecBridge.java > > > (right): > > > > > > > > > https://codereview.chromium.org/605883002/diff/1/media/base/android/java/src/... > > > media/base/android/java/src/org/chromium/media/MediaCodecBridge.java:242: > > > mediaCodec = MediaCodec.createByCodecName(decoderName); > > > Can you explain why CreateDecoderByType() will end up with buggy codecs? > > > I checked findMatchingCodecs() in OMXCodec.cpp, that's how > > CreateDecoderByType() > > > is implemented in native site. I saw that it also uses MediaCodecList to > > > retrieve a list of codecs, and return the list to ACodec, which normally > picks > > > the first one. > > > This implementation is pretty much the same as getDecoderNameForMime() here. > > So > > > I don't know why changing from CreateDecoderByType() to createByCodecName() > > > solves the issue. > > > > > > On 2014/09/26 18:01:37, AlexGlaznev wrote: > > > > On 2014/09/26 05:00:28, qinmin wrote: > > > > > Are you sure this will not cause any other issues? > > > > > > > > No, I am not 100% sure. But I know, that current way of codec selection is > > > > causing issues on majority of Samsung devices. > > > > > > > > > AFAIK, Calling createDecoderByType() will automatically switch to > software > > > > > decoder if hardware decoder is currently being used. However, if we call > > > > > createByCodecName() twice on the hardware decoder, the 2nd call will > fail. > > > > > > > > Yes, you are right - probably not on a second call, but after all HW codec > > > > instances will be used - probably after 7 - 12 calls. Current Android GTS > > > tests > > > > require at least 3 encoder and 5 decoder instances to run in parallel (all > > > > instances are created using createByCodecName). > > > > > > > > But the drawback of using createDecoderByType is that it can pick up non > HW > > > > accelerated or buggy codec (and this is what is happening for Samsung > > devices) > > > > or fall back to this bad codec after all available instances of HW codec > > will > > > be > > > > used. > > > > > > > > So I think there could be two possible solutions: > > > > > > > > 1. Continue to use CreateEncoder{Decoder}ByType, but change > > > > MediaCodecBridge::IsKnownUnaccelerated to obtain the name of the default > > codec > > > > (it will require opening codec instance with CreateEncoder{Decoder}ByType > > and > > > > calling MediaCodec.getName()) instead of the top codec in MediaCodecList. > > Then > > > > android_video_decoder_accelerator.cc will not try to create and use > > MediaCodec > > > > for devices with default OMX.google or OMX.SEC codecs. > > > > > > > > 2. Approach in this CL - for video decoder use CreateByCodecName and > expect > > > that > > > > it will fail after all HW codec instances will be used. > > > > > > > > Please advise what do you think will be a better choice. > > > > > > > > > > > > I am not sure, but I think the requirement that MediaCodecList contains the > > codec sorted in the order of preference will appear for L only (comment #25 to > > b/9735008). > > For KK devices the default codec (opened by createDecoderByType) may not be > the > > first one in MediaCodecList. > > > > For example for Galaxy S4 I9505 MediaCodecList contains following VP8 codecs: > > OMX.qcom.video.decoder.vp8, > > OMX.SEC.vp8.dec, > > OMX.google.vpx.decode > > so MediaCodecBridge.getCodecsInfo, which iterates through this list will > create > > CodecInfo with "OMX.qcom.video.decoder.vp8" for "video/x-vnd.on2.vp8" and > > IsKnownUnaccelerated will return false. > > However when later MediaCodec is created using CreateDecoderByType > > "OMX.SEC.vp8.dec" is actually created, which can not be used for surface > decoder > > (it will either fail or generate black output). > > > > However so far this is the only device I know which has this problem - for > other > > Samsung devices default video codec and top codec in MediaCodecList are the > > same. So if not that S4 I9505 changing MediaCodecBridge::IsKnownUnaccelerated > to > > report OMX.SEC. codec as unaccelerated will be enough. > > > > Probably to minimize the risk I can leave MediaCodecBridge.java untouched and > > only do this minimal change to MediaCodecBridge::IsKnownUnaccelerated and > leave > > I9505 in video decoder black list. > > ok, blacklist I9505 sounds good to me. If L will maintain codecs in sorted > order, then we should try using createDecoderByType(). Or otherwise, we will > have another if-else statement for different android versions that we don't know > whether it works. I9505 is already black listed, but there are so many different Samsung devices that I am afraid some of them may also have non ordered MediaCodecList. One more solution, which will allow to stay with createDecoderByType() is to change MediaCodecBridge::IsKnownUnaccelerated to request the name of default video codecs instead of the name of top codec in MediaCodecList. Default codec name can be obtained by opening codec for a given mime, calling MediaCodec.getName() and then closing the codec. It will require adding one extra function to MediaCodecBridge.java - private static CodecInfo[] getDefaultCodecsInfo(String mime) and will will work only for devices with JB MR2 and above. For devices with lower Android version we can continue to use top codec in MediacodecList. Are you ok with this approach?
On 2014/09/26 23:48:25, AlexGlaznev wrote: > On 2014/09/26 23:34:19, qinmin wrote: > > On 2014/09/26 23:19:07, AlexGlaznev wrote: > > > On 2014/09/26 22:45:55, qinmin wrote: > > > > > > > > > > https://codereview.chromium.org/605883002/diff/1/media/base/android/java/src/... > > > > File media/base/android/java/src/org/chromium/media/MediaCodecBridge.java > > > > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/605883002/diff/1/media/base/android/java/src/... > > > > media/base/android/java/src/org/chromium/media/MediaCodecBridge.java:242: > > > > mediaCodec = MediaCodec.createByCodecName(decoderName); > > > > Can you explain why CreateDecoderByType() will end up with buggy codecs? > > > > I checked findMatchingCodecs() in OMXCodec.cpp, that's how > > > CreateDecoderByType() > > > > is implemented in native site. I saw that it also uses MediaCodecList to > > > > retrieve a list of codecs, and return the list to ACodec, which normally > > picks > > > > the first one. > > > > This implementation is pretty much the same as getDecoderNameForMime() > here. > > > So > > > > I don't know why changing from CreateDecoderByType() to > createByCodecName() > > > > solves the issue. > > > > > > > > On 2014/09/26 18:01:37, AlexGlaznev wrote: > > > > > On 2014/09/26 05:00:28, qinmin wrote: > > > > > > Are you sure this will not cause any other issues? > > > > > > > > > > No, I am not 100% sure. But I know, that current way of codec selection > is > > > > > causing issues on majority of Samsung devices. > > > > > > > > > > > AFAIK, Calling createDecoderByType() will automatically switch to > > software > > > > > > decoder if hardware decoder is currently being used. However, if we > call > > > > > > createByCodecName() twice on the hardware decoder, the 2nd call will > > fail. > > > > > > > > > > Yes, you are right - probably not on a second call, but after all HW > codec > > > > > instances will be used - probably after 7 - 12 calls. Current Android > GTS > > > > tests > > > > > require at least 3 encoder and 5 decoder instances to run in parallel > (all > > > > > instances are created using createByCodecName). > > > > > > > > > > But the drawback of using createDecoderByType is that it can pick up non > > HW > > > > > accelerated or buggy codec (and this is what is happening for Samsung > > > devices) > > > > > or fall back to this bad codec after all available instances of HW codec > > > will > > > > be > > > > > used. > > > > > > > > > > So I think there could be two possible solutions: > > > > > > > > > > 1. Continue to use CreateEncoder{Decoder}ByType, but change > > > > > MediaCodecBridge::IsKnownUnaccelerated to obtain the name of the default > > > codec > > > > > (it will require opening codec instance with > CreateEncoder{Decoder}ByType > > > and > > > > > calling MediaCodec.getName()) instead of the top codec in > MediaCodecList. > > > Then > > > > > android_video_decoder_accelerator.cc will not try to create and use > > > MediaCodec > > > > > for devices with default OMX.google or OMX.SEC codecs. > > > > > > > > > > 2. Approach in this CL - for video decoder use CreateByCodecName and > > expect > > > > that > > > > > it will fail after all HW codec instances will be used. > > > > > > > > > > Please advise what do you think will be a better choice. > > > > > > > > > > > > > > > > I am not sure, but I think the requirement that MediaCodecList contains the > > > codec sorted in the order of preference will appear for L only (comment #25 > to > > > b/9735008). > > > For KK devices the default codec (opened by createDecoderByType) may not be > > the > > > first one in MediaCodecList. > > > > > > For example for Galaxy S4 I9505 MediaCodecList contains following VP8 > codecs: > > > OMX.qcom.video.decoder.vp8, > > > OMX.SEC.vp8.dec, > > > OMX.google.vpx.decode > > > so MediaCodecBridge.getCodecsInfo, which iterates through this list will > > create > > > CodecInfo with "OMX.qcom.video.decoder.vp8" for "video/x-vnd.on2.vp8" and > > > IsKnownUnaccelerated will return false. > > > However when later MediaCodec is created using CreateDecoderByType > > > "OMX.SEC.vp8.dec" is actually created, which can not be used for surface > > decoder > > > (it will either fail or generate black output). > > > > > > However so far this is the only device I know which has this problem - for > > other > > > Samsung devices default video codec and top codec in MediaCodecList are the > > > same. So if not that S4 I9505 changing > MediaCodecBridge::IsKnownUnaccelerated > > to > > > report OMX.SEC. codec as unaccelerated will be enough. > > > > > > Probably to minimize the risk I can leave MediaCodecBridge.java untouched > and > > > only do this minimal change to MediaCodecBridge::IsKnownUnaccelerated and > > leave > > > I9505 in video decoder black list. > > > > ok, blacklist I9505 sounds good to me. If L will maintain codecs in sorted > > order, then we should try using createDecoderByType(). Or otherwise, we will > > have another if-else statement for different android versions that we don't > know > > whether it works. > > I9505 is already black listed, but there are so many different Samsung devices > that I am afraid some of them may also have non ordered MediaCodecList. > One more solution, which will allow to stay with createDecoderByType() is to > change MediaCodecBridge::IsKnownUnaccelerated to request the name of default > video codecs instead of the name of top codec in MediaCodecList. Default codec > name can be obtained by opening codec for a given mime, calling > MediaCodec.getName() and then closing the codec. It will require adding one > extra function to MediaCodecBridge.java - > private static CodecInfo[] getDefaultCodecsInfo(String mime) and will will work > only for devices with JB MR2 and above. For devices with lower Android version > we can continue to use top codec in MediacodecList. Are you ok with this > approach? Sounds good. If MediaCodecList in the future supports codec priorities, we can wrap that functionality inside getDefaultCodecsInfo() to get the best available codec.
PTAL
lgtm % nit https://codereview.chromium.org/605883002/diff/40001/media/base/android/media... File media/base/android/media_codec_bridge.cc (right): https://codereview.chromium.org/605883002/diff/40001/media/base/android/media... media/base/android/media_codec_bridge.cc:212: if (codec_name.length() > 0) nit: {} needed for if statement that spans multiple lines
The CQ bit was checked by glaznev@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/605883002/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as c22cf74189cad2c0654c57ec076f2f31bef1cb9a
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/6e86e4f1154c5f26ea343b635d3e218a3495da7e Cr-Commit-Position: refs/heads/master@{#297667} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
