Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(17)

Issue 1479793002: Remove RegisterExternal{De,En}coder error codes. (Closed)

Created:
3 years, 9 months ago by pbos-webrtc
Modified:
3 years, 7 months ago
Reviewers:
noahric, stefan-webrtc
CC:
webrtc-reviews_webrtc.org, video-team_agora.io, yujie_mao (webrtc), zhengzhonghou_agora.io, stefan-webrtc, tterriberry_mozilla.com, andresp, the sun, perkj_webrtc, mflodman
Base URL:
https://chromium.googlesource.com/external/webrtc.git@master
Target Ref:
refs/pending/heads/master
Project:
webrtc
Visibility:
Public.

Description

Remove RegisterExternal{De,En}coder error codes. Also adds a RTC_CHECK in VideoReceiveStream that verifies that decoders aren't null, since this will attempt to deregister a codec which would previously fail with an obscure stack trace not indicating what actually was wrong. BUG=webrtc:5249 R=stefan@webrtc.org Committed: https://crrev.com/795dbe4e0fa78333d52fe39edb08a0b13a281c34 Cr-Commit-Position: refs/heads/master@{#10821}

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+37 lines, -58 lines) Patch
M webrtc/modules/video_coding/codec_database.h View 1 chunk +1 line, -1 line 0 comments Download
M webrtc/modules/video_coding/codec_database.cc View 1 chunk +1 line, -5 lines 0 comments Download
M webrtc/modules/video_coding/include/video_coding.h View 2 chunks +4 lines, -6 lines 0 comments Download
M webrtc/modules/video_coding/video_coding_impl.h View 2 chunks +6 lines, -6 lines 0 comments Download
M webrtc/modules/video_coding/video_coding_impl.cc View 2 chunks +8 lines, -7 lines 0 comments Download
M webrtc/modules/video_coding/video_coding_robustness_unittest.cc View 1 chunk +1 line, -3 lines 0 comments Download
M webrtc/modules/video_coding/video_receiver.cc View 1 chunk +4 lines, -6 lines 0 comments Download
M webrtc/modules/video_coding/video_receiver_unittest.cc View 1 chunk +1 line, -2 lines 0 comments Download
M webrtc/modules/video_coding/video_sender.cc View 2 chunks +4 lines, -5 lines 0 comments Download
M webrtc/modules/video_coding/video_sender_unittest.cc View 3 chunks +4 lines, -9 lines 0 comments Download
M webrtc/video/video_receive_stream.cc View 1 chunk +1 line, -0 lines 0 comments Download
M webrtc/video_engine/vie_channel.cc View 2 chunks +2 lines, -8 lines 0 comments Download

Messages

Total messages: 16 (5 generated)
pbos-webrtc
PTAL, Noah I believe this covers what you wanted.
3 years, 9 months ago (2015-11-26 10:15:54 UTC) #1
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1479793002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1479793002/1
3 years, 9 months ago (2015-11-26 10:16:12 UTC) #3
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
3 years, 9 months ago (2015-11-26 12:16:25 UTC) #5
stefan-webrtc
lgtm
3 years, 9 months ago (2015-11-26 14:02:52 UTC) #6
commit-bot: I haz the power
Patchset 1 (id:??) landed as https://crrev.com/795dbe4e0fa78333d52fe39edb08a0b13a281c34 Cr-Commit-Position: refs/heads/master@{#10821}
3 years, 9 months ago (2015-11-27 13:09:19 UTC) #10
pbos-webrtc
Committed patchset #1 (id:1) manually as 795dbe4e0fa78333d52fe39edb08a0b13a281c34 (presubmit successful).
3 years, 9 months ago (2015-11-27 13:09:20 UTC) #11
noahric
On 2015/11/27 13:09:20, pbos-webrtc wrote: > Committed patchset #1 (id:1) manually as > 795dbe4e0fa78333d52fe39edb08a0b13a281c34 (presubmit ...
3 years, 9 months ago (2015-11-30 17:50:33 UTC) #12
pbos-webrtc
If this is the case you can return WEBRTC_VIDEO_CODEC_FALLBACK_SOFTWARE for codecs supported in software. For ...
3 years, 9 months ago (2015-12-02 22:14:10 UTC) #13
noahric
On 2015/12/02 22:14:10, pbos-webrtc wrote: > If this is the case you can return WEBRTC_VIDEO_CODEC_FALLBACK_SOFTWARE ...
3 years, 9 months ago (2015-12-02 22:19:34 UTC) #14
pbos-webrtc
On 2015/12/02 22:19:34, noahric wrote: > On 2015/12/02 22:14:10, pbos-webrtc wrote: > > If this ...
3 years, 9 months ago (2015-12-02 22:26:38 UTC) #15
noahric
3 years, 9 months ago (2015-12-02 22:32:35 UTC) #16
Message was sent while issue was closed.
On 2015/12/02 22:26:38, pbos-webrtc wrote:
> On 2015/12/02 22:19:34, noahric wrote:
> > On 2015/12/02 22:14:10, pbos-webrtc wrote:
> > > If this is the case you can return WEBRTC_VIDEO_CODEC_FALLBACK_SOFTWARE
for
> > > codecs supported in software. For H264 we've more of a problem, since
> there's
> > no
> > > signal in WebRTC that permits an application to know that a codec has
become
> > > unavailable/initialization fails, so there's no way to tell the other side
> to
> > > stop sending it. Without this signal being available (which is how it
really
> > > should be handled), I prefer not implementing a silent breakage.
> > 
> > You can't return that from the factory's CreateDecoder method, so do you
mean
> > factories should create a decoder object, even if it can't support it, and
> then
> > eventually return fallback software? My overall problem isn't that it's
silent
> > or not, it's that it *can't* be silent now, meaning webrtc has decided that
> > every app that uses it must crash in this case (without a hacky workaround).
I
> > don't think Chrome would appreciate that, if it was possible to fail on
chrome
> > (it would if the mac h264 was enabled and also failed in some dynamic way).
> > 
> > Agreed about the other limitation. There are probably a few sensible ways of
> > doing it (something rtcp/fir-like that says "I don't support this codec"; or
> an
> > event on the <media engine? something> that says "I can no longer receive
this
> > codec"; or that + an event on PC that says "my offer has changed, please
> > renegotiate"). Is it worth opening a bug? Again, though, I'd prefer options
> > other than "forces a crash" for this :)
> 
> Maybe we can at least fallback to software for decoders supported in software
> and do a null-decoder object for non-supported ones (and spam LS_ERROR) until
if
> we'd have a way to support that. Can you reopen the bug for this and put our
> discussion there?

Moved it over and re-opened. Thanks!

Powered by Google App Engine
This is Rietveld 408576698