Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
4 years, 4 months ago by sakal Modified:
4 years, 2 months ago Reviewers:
magjed_webrtc, AlexG, stefan-webrtc CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionMake MediaCodecEncoder fallback to a software encoder on failure.
This should allow us to enable Intel HW VP8 encoder again.
BUG=webrtc:6232
,b/30947951
Committed: https://crrev.com/327e9d0821274a2f519ff261028b4044e4984d40
Cr-Commit-Position: refs/heads/master@{#14552}
Patch Set 1 #Patch Set 2 : Drop one frame if the fallback encoder doesn't support native frames. #Patch Set 3 : Don't crash if software encoder is not supported. #Patch Set 4 : Remove empty lines. #
Total comments: 7
Patch Set 5 : Return WEBRTC_VIDEO_CODEC_FALLBACK_SOFTWARE immediately. #
Total comments: 8
Patch Set 6 : Rebase. #Patch Set 7 : Changes according to AlexG's comments. #Patch Set 8 : Fallback instead of trying to reset the encoder. #
Total comments: 20
Patch Set 9 : Changes according to AlexG's comments. #2 #
Total comments: 4
Patch Set 10 : Rebase. #Patch Set 11 : Changes according to AlexG's comments. #3 #
Total comments: 10
Patch Set 12 : Changes according to Stefan's comments. #
Created: 4 years, 2 months ago
Messages
Total messages: 47 (23 generated)
Description was changed from ========== Make MediaCodecEncoder fallback to a software encoder if it is reset too many times. ========== to ========== Make MediaCodecEncoder fallback to a software encoder if it is reset too many times. This should allow us to enable Intel HW VP8 encoder again. ==========
sakal@webrtc.org changed reviewers: + magjed@webrtc.org
PTAL
Description was changed from ========== Make MediaCodecEncoder fallback to a software encoder if it is reset too many times. This should allow us to enable Intel HW VP8 encoder again. ========== to ========== Make MediaCodecEncoder fallback to a software encoder if it is reset too many times. This should allow us to enable Intel HW VP8 encoder again. BUG=b/30947951 ==========
https://codereview.webrtc.org/2263043003/diff/60001/webrtc/api/android/jni/an... File webrtc/api/android/jni/androidmediaencoder_jni.cc (right): https://codereview.webrtc.org/2263043003/diff/60001/webrtc/api/android/jni/an... webrtc/api/android/jni/androidmediaencoder_jni.cc:91: const int kMaxEncoderResetsBeforeFallback = 3; Now it will fall back to software the fourth time, i.e. when we have failed to reset the HW encoder 3 times. I think it is enough to only try resetting the HW encoder one time, so I would like to change this value to 1. https://codereview.webrtc.org/2263043003/diff/60001/webrtc/api/android/jni/an... webrtc/api/android/jni/androidmediaencoder_jni.cc:508: software_fallback_needed_ = true; Is it possible to remove |software_fallback_needed_| and check the return value everywhere instead? I.e. if (!ResetCodecOnCodecThread()) return WEBRTC_VIDEO_CODEC_FALLBACK_SOFTWARE; or return ResetCodecOnCodecThread() ? WEBRTC_VIDEO_CODEC_ERROR : WEBRTC_VIDEO_CODEC_FALLBACK_SOFTWARE; https://codereview.webrtc.org/2263043003/diff/60001/webrtc/video/video_encode... File webrtc/video/video_encoder.cc (right): https://codereview.webrtc.org/2263043003/diff/60001/webrtc/video/video_encode... webrtc/video/video_encoder.cc:168: LOG(LS_WARNING) << "Fallback encoder doesn't support native frames, " So what will happen when the HW encoder fails on Android and we use native frames from the camera? Who is responsible for calling NativeToI420?
https://codereview.webrtc.org/2263043003/diff/60001/webrtc/api/android/jni/an... File webrtc/api/android/jni/androidmediaencoder_jni.cc (right): https://codereview.webrtc.org/2263043003/diff/60001/webrtc/api/android/jni/an... webrtc/api/android/jni/androidmediaencoder_jni.cc:91: const int kMaxEncoderResetsBeforeFallback = 3; On 2016/08/23 12:09:50, magjed_webrtc wrote: > Now it will fall back to software the fourth time, i.e. when we have failed to > reset the HW encoder 3 times. I think it is enough to only try resetting the HW > encoder one time, so I would like to change this value to 1. Resetting the encoder is fairly quick so 3 resets shouldn't take a very long time. Given that this value doesn't reset after a successful reset, I would rather keep it at 3 in case the encoder fails for example twice during a call. https://codereview.webrtc.org/2263043003/diff/60001/webrtc/api/android/jni/an... webrtc/api/android/jni/androidmediaencoder_jni.cc:508: software_fallback_needed_ = true; On 2016/08/23 12:09:50, magjed_webrtc wrote: > Is it possible to remove |software_fallback_needed_| and check the return value > everywhere instead? I.e. > if (!ResetCodecOnCodecThread()) > return WEBRTC_VIDEO_CODEC_FALLBACK_SOFTWARE; > or > return ResetCodecOnCodecThread() > ? WEBRTC_VIDEO_CODEC_ERROR > : WEBRTC_VIDEO_CODEC_FALLBACK_SOFTWARE; There would be some trouble. The codec might not be initialized after ResetCodecOnCodecThread fails. This makes some Java methods throw exceptions when for example SetRatesOnCodecThread gets called. https://codereview.webrtc.org/2263043003/diff/60001/webrtc/video/video_encode... File webrtc/video/video_encoder.cc (right): https://codereview.webrtc.org/2263043003/diff/60001/webrtc/video/video_encode... webrtc/video/video_encoder.cc:168: LOG(LS_WARNING) << "Fallback encoder doesn't support native frames, " On 2016/08/23 12:09:50, magjed_webrtc wrote: > So what will happen when the HW encoder fails on Android and we use native > frames from the camera? Who is responsible for calling NativeToI420? When fallback_encoder_ is initialized, SupportsNativeHandle in this class will start to forward the calls to the fallback encoder instead. This causes VideoSender to start converting the frames for the encoder if the fallback encoder doesn't support native frames.
Description was changed from ========== Make MediaCodecEncoder fallback to a software encoder if it is reset too many times. This should allow us to enable Intel HW VP8 encoder again. BUG=b/30947951 ========== to ========== Make MediaCodecEncoder fallback to a software encoder if it is reset too many times. This should allow us to enable Intel HW VP8 encoder again. BUG=webrtc:6232,b/30947951 ==========
I made the methods return WEBRTC_VIDEO_CODEC_FALLBACK_SOFTWARE sooner as discussed offline.
lgtm https://codereview.webrtc.org/2263043003/diff/60001/webrtc/video/video_encode... File webrtc/video/video_encoder.cc (right): https://codereview.webrtc.org/2263043003/diff/60001/webrtc/video/video_encode... webrtc/video/video_encoder.cc:168: LOG(LS_WARNING) << "Fallback encoder doesn't support native frames, " On 2016/08/23 12:26:51, sakal wrote: > On 2016/08/23 12:09:50, magjed_webrtc wrote: > > So what will happen when the HW encoder fails on Android and we use native > > frames from the camera? Who is responsible for calling NativeToI420? > > When fallback_encoder_ is initialized, SupportsNativeHandle in this class will > start to forward the calls to the fallback encoder instead. > > This causes VideoSender to start converting the frames for the encoder if the > fallback encoder doesn't support native frames. Acknowledged.
glaznev@webrtc.org changed reviewers: + glaznev@webrtc.org
It may worth to check similar fallback decoder implementation and make encoder implementation similar. https://codereview.webrtc.org/2263043003/diff/80001/webrtc/api/android/jni/an... File webrtc/api/android/jni/androidmediaencoder_jni.cc (right): https://codereview.webrtc.org/2263043003/diff/80001/webrtc/api/android/jni/an... webrtc/api/android/jni/androidmediaencoder_jni.cc:91: const int kMaxEncoderResetsBeforeFallback = 3; I don't think this is necessary - similarly to decoder we can fallback as soon as we detected any HW encoder error. This will make switch faster. https://codereview.webrtc.org/2263043003/diff/80001/webrtc/api/android/jni/an... webrtc/api/android/jni/androidmediaencoder_jni.cc:289: bool software_fallback_needed_; suggest to rename it to sw_fallback_required_ so we use same variable names both for decoder and encoder https://codereview.webrtc.org/2263043003/diff/80001/webrtc/api/android/jni/an... webrtc/api/android/jni/androidmediaencoder_jni.cc:883: if (software_fallback_needed_) { is this needed? https://codereview.webrtc.org/2263043003/diff/80001/webrtc/api/android/jni/an... webrtc/api/android/jni/androidmediaencoder_jni.cc:941: CHECK_EXCEPTION(jni); Replace all CHECK_EXCEPTION(jni) with CheckException(jni) and fallback to SW if Java exception happens - similarly to decoder implementation
Patchset #8 (id:140001) has been deleted
I find it hard to properly test this CL because there are a lot of code paths only used in error conditions. I tested it by manually causing exception in the same place as Intel VP8 encoder failed using different encoders. If codec with SW encoder support was used, encoding seamlessly continued on the SW encoder. If codec without SW encoder support was used, no more frames were delivered. https://codereview.webrtc.org/2263043003/diff/80001/webrtc/api/android/jni/an... File webrtc/api/android/jni/androidmediaencoder_jni.cc (right): https://codereview.webrtc.org/2263043003/diff/80001/webrtc/api/android/jni/an... webrtc/api/android/jni/androidmediaencoder_jni.cc:91: const int kMaxEncoderResetsBeforeFallback = 3; On 2016/08/23 22:50:31, AlexG wrote: > I don't think this is necessary - similarly to decoder we can fallback as soon > as we detected any HW encoder error. This will make switch faster. Done. https://codereview.webrtc.org/2263043003/diff/80001/webrtc/api/android/jni/an... webrtc/api/android/jni/androidmediaencoder_jni.cc:289: bool software_fallback_needed_; On 2016/08/23 22:50:31, AlexG wrote: > suggest to rename it to sw_fallback_required_ so we use same variable names both > for decoder and encoder Done. https://codereview.webrtc.org/2263043003/diff/80001/webrtc/api/android/jni/an... webrtc/api/android/jni/androidmediaencoder_jni.cc:883: if (software_fallback_needed_) { On 2016/08/23 22:50:31, AlexG wrote: > is this needed? I guess not. I just added it as a safety measure. https://codereview.webrtc.org/2263043003/diff/80001/webrtc/api/android/jni/an... webrtc/api/android/jni/androidmediaencoder_jni.cc:941: CHECK_EXCEPTION(jni); On 2016/08/23 22:50:31, AlexG wrote: > Replace all CHECK_EXCEPTION(jni) with CheckException(jni) and fallback to SW if > Java exception happens - similarly to decoder implementation Done.
Description was changed from ========== Make MediaCodecEncoder fallback to a software encoder if it is reset too many times. This should allow us to enable Intel HW VP8 encoder again. BUG=webrtc:6232,b/30947951 ========== to ========== Make MediaCodecEncoder fallback to a software encoder on failure. This should allow us to enable Intel HW VP8 encoder again. BUG=webrtc:6232,b/30947951 ==========
https://codereview.webrtc.org/2263043003/diff/160001/webrtc/api/android/jni/a... File webrtc/api/android/jni/androidmediaencoder_jni.cc (right): https://codereview.webrtc.org/2263043003/diff/160001/webrtc/api/android/jni/a... webrtc/api/android/jni/androidmediaencoder_jni.cc:383: check sw_fallback_required_ flag here https://codereview.webrtc.org/2263043003/diff/160001/webrtc/api/android/jni/a... webrtc/api/android/jni/androidmediaencoder_jni.cc:554: return WEBRTC_VIDEO_CODEC_ERROR; fallback to sw encoder here. Still return error, but set sw_fallback_required_ = true https://codereview.webrtc.org/2263043003/diff/160001/webrtc/api/android/jni/a... webrtc/api/android/jni/androidmediaencoder_jni.cc:559: return WEBRTC_VIDEO_CODEC_FALLBACK_SOFTWARE; return WEBRTC_VIDEO_CODEC_ERROR? In decoder WEBRTC_VIDEO_CODEC_FALLBACK_SOFTWAR is used in Decode() function only. Not sure if same applies to encoder https://codereview.webrtc.org/2263043003/diff/160001/webrtc/api/android/jni/a... webrtc/api/android/jni/androidmediaencoder_jni.cc:573: return WEBRTC_VIDEO_CODEC_ERROR; set sw_fallback_required_ = true https://codereview.webrtc.org/2263043003/diff/160001/webrtc/api/android/jni/a... webrtc/api/android/jni/androidmediaencoder_jni.cc:587: LOG(LS_ERROR) << "Wrong color format."; set sw_fallback_required_ = true https://codereview.webrtc.org/2263043003/diff/160001/webrtc/api/android/jni/a... webrtc/api/android/jni/androidmediaencoder_jni.cc:828: sw_fallback_required_ = true; We can not fallback to SW encoder for H.264 codec - may need to implement something like MediaCodecVideoDecoder::ProcessHWErrorOnCodecThread() here https://codereview.webrtc.org/2263043003/diff/160001/webrtc/api/android/jni/a... webrtc/api/android/jni/androidmediaencoder_jni.cc:903: return WEBRTC_VIDEO_CODEC_FALLBACK_SOFTWARE; Return error here? https://codereview.webrtc.org/2263043003/diff/160001/webrtc/api/android/jni/a... webrtc/api/android/jni/androidmediaencoder_jni.cc:916: return WEBRTC_VIDEO_CODEC_FALLBACK_SOFTWARE; Return WEBRTC_VIDEO_CODEC_OK here? WEBRTC_VIDEO_CODEC_FALLBACK_SOFTWARE should be returned by decode() function only? https://codereview.webrtc.org/2263043003/diff/160001/webrtc/api/android/jni/a... webrtc/api/android/jni/androidmediaencoder_jni.cc:940: sw_fallback_required_ = true; ditto: Return WEBRTC_VIDEO_CODEC_OK here, but keep sw_fallback_required_ = true? https://codereview.webrtc.org/2263043003/diff/160001/webrtc/api/android/jni/a... webrtc/api/android/jni/androidmediaencoder_jni.cc:1123: sw_fallback_required_ = true; You can not fallback to SW for H.264 encoder - we may need to still keep reset logic
Patchset #9 (id:180001) has been deleted
https://codereview.webrtc.org/2263043003/diff/160001/webrtc/api/android/jni/a... File webrtc/api/android/jni/androidmediaencoder_jni.cc (right): https://codereview.webrtc.org/2263043003/diff/160001/webrtc/api/android/jni/a... webrtc/api/android/jni/androidmediaencoder_jni.cc:383: On 2016/08/31 00:25:57, AlexG wrote: > check sw_fallback_required_ flag here Done. https://codereview.webrtc.org/2263043003/diff/160001/webrtc/api/android/jni/a... webrtc/api/android/jni/androidmediaencoder_jni.cc:554: return WEBRTC_VIDEO_CODEC_ERROR; On 2016/08/31 00:25:57, AlexG wrote: > fallback to sw encoder here. Still return error, but set sw_fallback_required_ = > true Done. https://codereview.webrtc.org/2263043003/diff/160001/webrtc/api/android/jni/a... webrtc/api/android/jni/androidmediaencoder_jni.cc:559: return WEBRTC_VIDEO_CODEC_FALLBACK_SOFTWARE; On 2016/08/31 00:25:58, AlexG wrote: > return WEBRTC_VIDEO_CODEC_ERROR? In decoder WEBRTC_VIDEO_CODEC_FALLBACK_SOFTWAR > is used in Decode() function only. Not sure if same applies to encoder Done. https://codereview.webrtc.org/2263043003/diff/160001/webrtc/api/android/jni/a... webrtc/api/android/jni/androidmediaencoder_jni.cc:573: return WEBRTC_VIDEO_CODEC_ERROR; On 2016/08/31 00:25:57, AlexG wrote: > set sw_fallback_required_ = true Done. https://codereview.webrtc.org/2263043003/diff/160001/webrtc/api/android/jni/a... webrtc/api/android/jni/androidmediaencoder_jni.cc:587: LOG(LS_ERROR) << "Wrong color format."; On 2016/08/31 00:25:57, AlexG wrote: > set sw_fallback_required_ = true Done. https://codereview.webrtc.org/2263043003/diff/160001/webrtc/api/android/jni/a... webrtc/api/android/jni/androidmediaencoder_jni.cc:828: sw_fallback_required_ = true; On 2016/08/31 00:25:57, AlexG wrote: > We can not fallback to SW encoder for H.264 codec - may need to implement > something like MediaCodecVideoDecoder::ProcessHWErrorOnCodecThread() here Done. https://codereview.webrtc.org/2263043003/diff/160001/webrtc/api/android/jni/a... webrtc/api/android/jni/androidmediaencoder_jni.cc:903: return WEBRTC_VIDEO_CODEC_FALLBACK_SOFTWARE; On 2016/08/31 00:25:58, AlexG wrote: > Return error here? Done. https://codereview.webrtc.org/2263043003/diff/160001/webrtc/api/android/jni/a... webrtc/api/android/jni/androidmediaencoder_jni.cc:916: return WEBRTC_VIDEO_CODEC_FALLBACK_SOFTWARE; On 2016/08/31 00:25:58, AlexG wrote: > Return WEBRTC_VIDEO_CODEC_OK here? WEBRTC_VIDEO_CODEC_FALLBACK_SOFTWARE should > be returned by decode() function only? Done. https://codereview.webrtc.org/2263043003/diff/160001/webrtc/api/android/jni/a... webrtc/api/android/jni/androidmediaencoder_jni.cc:940: sw_fallback_required_ = true; On 2016/08/31 00:25:58, AlexG wrote: > ditto: Return WEBRTC_VIDEO_CODEC_OK here, but keep sw_fallback_required_ = true? Done. https://codereview.webrtc.org/2263043003/diff/160001/webrtc/api/android/jni/a... webrtc/api/android/jni/androidmediaencoder_jni.cc:1123: sw_fallback_required_ = true; On 2016/08/31 00:25:58, AlexG wrote: > You can not fallback to SW for H.264 encoder - we may need to still keep reset > logic Done.
Alexander, can you take a look at this CL please?
On 2016/09/08 08:43:39, sakal wrote: > Alexander, can you take a look at this CL please? Sami, sorry for the delay. Will try to finish it today
lgtm https://codereview.webrtc.org/2263043003/diff/200001/webrtc/api/android/jni/a... File webrtc/api/android/jni/androidmediaencoder_jni.cc (right): https://codereview.webrtc.org/2263043003/diff/200001/webrtc/api/android/jni/a... webrtc/api/android/jni/androidmediaencoder_jni.cc:396: return WEBRTC_VIDEO_CODEC_ERROR; WEBRTC_VIDEO_CODEC_OK - similarly to InitDecode() https://codereview.webrtc.org/2263043003/diff/200001/webrtc/api/android/jni/a... webrtc/api/android/jni/androidmediaencoder_jni.cc:536: return false; nit: not necessary return here https://codereview.webrtc.org/2263043003/diff/200001/webrtc/api/android/jni/a... webrtc/api/android/jni/androidmediaencoder_jni.cc:543: return WEBRTC_VIDEO_CODEC_ERROR; WEBRTC_VIDEO_CODEC_OK? https://codereview.webrtc.org/2263043003/diff/200001/webrtc/api/android/jni/a... webrtc/api/android/jni/androidmediaencoder_jni.cc:727: ProcessHWErrorOnCodecThread(true /* tryResetIfFallbackUnavailable */); Create helper function for ProcessHWErrorOnCodecThread(true /* tryResetIfFallbackUnavailable */); return sw_fallback_required_ ? WEBRTC_VIDEO_CODEC_FALLBACK_SOFTWARE : WEBRTC_VIDEO_CODEC_ERROR; - this is duplicated in several places
The CQ bit was checked by sakal@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/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by sakal@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from magjed@webrtc.org, glaznev@webrtc.org Link to the patchset: https://codereview.webrtc.org/2263043003/#ps240001 (title: "Changes according to AlexG's comments. #3")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/8286)
sakal@webrtc.org changed reviewers: + tommi@webrtc.org
Additional approval needed. Tommi, PTAL.
sakal@webrtc.org changed reviewers: + stefan@webrtc.org - tommi@webrtc.org
Stefan, maybe you could review this instead of tommi since he is busy?
Patchset #12 (id:260001) has been deleted
My main worry about this CL is that it doesn't add any tests, although the functionality it adds is non-trivial. What can we do to improve that? https://codereview.webrtc.org/2263043003/diff/240001/webrtc/api/android/jni/a... File webrtc/api/android/jni/androidmediaencoder_jni.cc (right): https://codereview.webrtc.org/2263043003/diff/240001/webrtc/api/android/jni/a... webrtc/api/android/jni/androidmediaencoder_jni.cc:139: bool ProcessHWErrorOnCodecThread(bool tryResetIfFallbackUnavailable); No camel case variable names. https://codereview.webrtc.org/2263043003/diff/240001/webrtc/api/android/jni/a... webrtc/api/android/jni/androidmediaencoder_jni.cc:674: if (sw_fallback_required_) { Optionally remove {} https://codereview.webrtc.org/2263043003/diff/240001/webrtc/api/android/jni/a... webrtc/api/android/jni/androidmediaencoder_jni.cc:703: if (!ProcessHWErrorOnCodecThread(true /* tryResetIfFallbackUnavailable */)) {} https://codereview.webrtc.org/2263043003/diff/240001/webrtc/api/android/jni/a... webrtc/api/android/jni/androidmediaencoder_jni.cc:965: if (sw_fallback_required_) { You may remove {} https://codereview.webrtc.org/2263043003/diff/240001/webrtc/video/video_encod... File webrtc/video/video_encoder.cc (right): https://codereview.webrtc.org/2263043003/diff/240001/webrtc/video/video_encod... webrtc/video/video_encoder.cc:37: bool VideoEncoder::IsSupported(EncoderType codec_type) { Should this be IsSupportedSoftware()?
Patchset #12 (id:280001) has been deleted
The problem is this code is really hard to test because it only triggers when there is an error in MediaCodecVideoEncoder. So to test it, we would have to create some kind of mock MediaCodecVideoEncoder that throws errors at our will. I have tested the code manully by throwing an error in dequeueOutputBuffer which is something that happened in a real bug. https://codereview.webrtc.org/2263043003/diff/240001/webrtc/api/android/jni/a... File webrtc/api/android/jni/androidmediaencoder_jni.cc (right): https://codereview.webrtc.org/2263043003/diff/240001/webrtc/api/android/jni/a... webrtc/api/android/jni/androidmediaencoder_jni.cc:139: bool ProcessHWErrorOnCodecThread(bool tryResetIfFallbackUnavailable); On 2016/10/06 09:27:47, stefan-webrtc (holmer) wrote: > No camel case variable names. Done. https://codereview.webrtc.org/2263043003/diff/240001/webrtc/api/android/jni/a... webrtc/api/android/jni/androidmediaencoder_jni.cc:674: if (sw_fallback_required_) { On 2016/10/06 09:27:47, stefan-webrtc (holmer) wrote: > Optionally remove {} Done. https://codereview.webrtc.org/2263043003/diff/240001/webrtc/api/android/jni/a... webrtc/api/android/jni/androidmediaencoder_jni.cc:703: if (!ProcessHWErrorOnCodecThread(true /* tryResetIfFallbackUnavailable */)) On 2016/10/06 09:27:47, stefan-webrtc (holmer) wrote: > {} Done. https://codereview.webrtc.org/2263043003/diff/240001/webrtc/api/android/jni/a... webrtc/api/android/jni/androidmediaencoder_jni.cc:965: if (sw_fallback_required_) { On 2016/10/06 09:27:47, stefan-webrtc (holmer) wrote: > You may remove {} Done. https://codereview.webrtc.org/2263043003/diff/240001/webrtc/video/video_encod... File webrtc/video/video_encoder.cc (right): https://codereview.webrtc.org/2263043003/diff/240001/webrtc/video/video_encod... webrtc/video/video_encoder.cc:37: bool VideoEncoder::IsSupported(EncoderType codec_type) { On 2016/10/06 09:27:47, stefan-webrtc (holmer) wrote: > Should this be IsSupportedSoftware()? This is class is kind of implicitly software based. For example VideoEncoder::Create only creates software encoders. Changed it because I don't really care either way.
On 2016/10/06 12:10:03, sakal wrote: > The problem is this code is really hard to test because it only triggers when > there is an error in MediaCodecVideoEncoder. So to test it, we would have to > create some kind of mock MediaCodecVideoEncoder that throws errors at our will. > > I have tested the code manully by throwing an error in dequeueOutputBuffer which > is something that happened in a real bug. Ack. I think it would be a good idea to plan form improved testability of this then. We're looking at 1400 lines of codes without a single test, which makes me a bit anxious... :) > > https://codereview.webrtc.org/2263043003/diff/240001/webrtc/api/android/jni/a... > File webrtc/api/android/jni/androidmediaencoder_jni.cc (right): > > https://codereview.webrtc.org/2263043003/diff/240001/webrtc/api/android/jni/a... > webrtc/api/android/jni/androidmediaencoder_jni.cc:139: bool > ProcessHWErrorOnCodecThread(bool tryResetIfFallbackUnavailable); > On 2016/10/06 09:27:47, stefan-webrtc (holmer) wrote: > > No camel case variable names. > > Done. > > https://codereview.webrtc.org/2263043003/diff/240001/webrtc/api/android/jni/a... > webrtc/api/android/jni/androidmediaencoder_jni.cc:674: if > (sw_fallback_required_) { > On 2016/10/06 09:27:47, stefan-webrtc (holmer) wrote: > > Optionally remove {} > > Done. > > https://codereview.webrtc.org/2263043003/diff/240001/webrtc/api/android/jni/a... > webrtc/api/android/jni/androidmediaencoder_jni.cc:703: if > (!ProcessHWErrorOnCodecThread(true /* tryResetIfFallbackUnavailable */)) > On 2016/10/06 09:27:47, stefan-webrtc (holmer) wrote: > > {} > > Done. > > https://codereview.webrtc.org/2263043003/diff/240001/webrtc/api/android/jni/a... > webrtc/api/android/jni/androidmediaencoder_jni.cc:965: if > (sw_fallback_required_) { > On 2016/10/06 09:27:47, stefan-webrtc (holmer) wrote: > > You may remove {} > > Done. > > https://codereview.webrtc.org/2263043003/diff/240001/webrtc/video/video_encod... > File webrtc/video/video_encoder.cc (right): > > https://codereview.webrtc.org/2263043003/diff/240001/webrtc/video/video_encod... > webrtc/video/video_encoder.cc:37: bool VideoEncoder::IsSupported(EncoderType > codec_type) { > On 2016/10/06 09:27:47, stefan-webrtc (holmer) wrote: > > Should this be IsSupportedSoftware()? > > This is class is kind of implicitly software based. For example > VideoEncoder::Create only creates software encoders. Changed it because I don't > really care either way.
lgtm
The CQ bit was checked by sakal@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from magjed@webrtc.org, glaznev@webrtc.org Link to the patchset: https://codereview.webrtc.org/2263043003/#ps300001 (title: "Changes according to Stefan's comments.")
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.
Description was changed from ========== Make MediaCodecEncoder fallback to a software encoder on failure. This should allow us to enable Intel HW VP8 encoder again. BUG=webrtc:6232,b/30947951 ========== to ========== Make MediaCodecEncoder fallback to a software encoder on failure. This should allow us to enable Intel HW VP8 encoder again. BUG=webrtc:6232,b/30947951 ==========
Message was sent while issue was closed.
Committed patchset #12 (id:300001)
Message was sent while issue was closed.
Description was changed from ========== Make MediaCodecEncoder fallback to a software encoder on failure. This should allow us to enable Intel HW VP8 encoder again. BUG=webrtc:6232,b/30947951 ========== to ========== Make MediaCodecEncoder fallback to a software encoder on failure. This should allow us to enable Intel HW VP8 encoder again. BUG=webrtc:6232,b/30947951 Committed: https://crrev.com/327e9d0821274a2f519ff261028b4044e4984d40 Cr-Commit-Position: refs/heads/master@{#14552} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/327e9d0821274a2f519ff261028b4044e4984d40 Cr-Commit-Position: refs/heads/master@{#14552} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
