Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2091923002:
Create a null decoder for h264 when not supported, instead of crashing. (Closed)
|
Created:
4 years, 6 months ago by noahric Modified:
4 years, 5 months ago CC:
webrtc-reviews_webrtc.org, video-team_agora.io, yujie_mao (webrtc), zhengzhonghou_agora.io, stefan-webrtc, tterriberry_mozilla.com, pbos-webrtc, perkj_webrtc, mflodman Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionCreate a null decoder for h264 when not supported, instead of crashing.
BUG=
NOTRY=true
Committed: https://crrev.com/0dacbf5f62644c4770496d883721666d98d8a63e
Cr-Commit-Position: refs/heads/master@{#13311}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Removed DCHECK and cleared up comment. #Messages
Total messages: 23 (12 generated)
noahric@chromium.org changed reviewers: + mflodman@webrtc.org, pbos@chromium.org
noahric@chromium.org changed reviewers: + mflodman@webrtc.org, pbos@chromium.org
Hey guys, let me know what you think about this. As written, the decoder creation around h264 can be pretty crashy, and it may be worth being a bit more resilient about it. I tried writing a unit test, but ran into a few things: 1) There's no way to disable h264 everywhere, just the ffmpeg version 2) Even if there was, the code around that is very static/global, and it could make running tests in parallel breakable 3) The cleaner ways of doing it all seemed pretty complicated, e.g. adding some type of supported or disabled codecs flag to the fallback wrapper. If you have any ideas, let me know.
Hey guys, let me know what you think about this. As written, the decoder creation around h264 can be pretty crashy, and it may be worth being a bit more resilient about it. I tried writing a unit test, but ran into a few things: 1) There's no way to disable h264 everywhere, just the ffmpeg version 2) Even if there was, the code around that is very static/global, and it could make running tests in parallel breakable 3) The cleaner ways of doing it all seemed pretty complicated, e.g. adding some type of supported or disabled codecs flag to the fallback wrapper. If you have any ideas, let me know.
pbos@webrtc.org changed reviewers: + pbos@webrtc.org
lgtm https://codereview.webrtc.org/2091923002/diff/1/webrtc/video/video_decoder.cc File webrtc/video/video_decoder.cc (right): https://codereview.webrtc.org/2091923002/diff/1/webrtc/video/video_decoder.cc... webrtc/video/video_decoder.cc:32: RTC_DCHECK(H264Decoder::IsSupported()); Remove this DCHECK.
https://codereview.webrtc.org/2091923002/diff/1/webrtc/video/video_decoder.cc File webrtc/video/video_decoder.cc (right): https://codereview.webrtc.org/2091923002/diff/1/webrtc/video/video_decoder.cc... webrtc/video/video_decoder.cc:32: RTC_DCHECK(H264Decoder::IsSupported()); On 2016/06/26 19:15:59, pbos-webrtc wrote: > Remove this DCHECK. Done. Also changed the log line slightly, since it may not be a software fallback as written (H264Decoder::Create would create a hardware backed decoder on e.g. iOS).
The CQ bit was checked by noahric@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pbos@webrtc.org Link to the patchset: https://codereview.webrtc.org/2091923002/#ps20001 (title: "Removed DCHECK and cleared up comment.")
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: android_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/14527)
The CQ bit was checked by noahric@chromium.org
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: android_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/14530)
Description was changed from ========== Create a null decoder for h264 when not supported, instead of crashing. BUG= ========== to ========== Create a null decoder for h264 when not supported, instead of crashing. BUG= NOTRY=true ==========
The CQ bit was checked by pbos@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.
Description was changed from ========== Create a null decoder for h264 when not supported, instead of crashing. BUG= NOTRY=true ========== to ========== Create a null decoder for h264 when not supported, instead of crashing. BUG= NOTRY=true ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Create a null decoder for h264 when not supported, instead of crashing. BUG= NOTRY=true ========== to ========== Create a null decoder for h264 when not supported, instead of crashing. BUG= NOTRY=true Committed: https://crrev.com/0dacbf5f62644c4770496d883721666d98d8a63e Cr-Commit-Position: refs/heads/master@{#13311} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/0dacbf5f62644c4770496d883721666d98d8a63e Cr-Commit-Position: refs/heads/master@{#13311} |
