Remove RegisterExternal{De,En}coder error codes.

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}

[pbos-webrtc, 2015-11-26 10:15:54]

PTAL, Noah I believe this covers what you wanted.

[pbos-webrtc, 2015-11-26 10:16:00]

The CQ bit was checked by pbos@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2015-11-26 10:16:12]

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

[commit-bot: I haz the power, 2015-11-26 12:16:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-26 12:16:25]

Dry run: Try jobs failed on following builders:
  android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[stefan-webrtc, 2015-11-26 14:02:52]

lgtm

[pbos-webrtc, 2015-11-27 12:46:24]

Description was changed from

==========
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=noahric@chromium.org, stefan@webrtc.org
==========

to

==========
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
==========

[pbos-webrtc, 2015-11-27 13:09:18]

Description was changed from

==========
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
==========

to

==========
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://chromium.googlesource.com/external/webrtc/+/795dbe4e0fa78333d52fe39ed...
==========

[commit-bot: I haz the power, 2015-11-27 13:09:18]

Description was changed from

==========
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
==========

to

==========
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}
==========

[commit-bot: I haz the power, 2015-11-27 13:09:19]

Patchset 1 (id:??) landed as
https://crrev.com/795dbe4e0fa78333d52fe39edb08a0b13a281c34
Cr-Commit-Position: refs/heads/master@{#10821}

[pbos-webrtc, 2015-11-27 13:09:20]

Committed patchset #1 (id:1) manually as
795dbe4e0fa78333d52fe39edb08a0b13a281c34 (presubmit successful).

[noahric, 2015-11-30 17:50:33]

On 2015/11/27 13:09:20, pbos-webrtc wrote:
> Committed patchset #1 (id:1) manually as
> 795dbe4e0fa78333d52fe39edb08a0b13a281c34 (presubmit successful).

Definitely better than the current crash pattern :) I still think you should
consider also changing wrve2 to allow a null decoder and just skip creating a
stream for it. Reason is that, on Android at least, you can't actually be
certain up front that a Decoder will be creatable, since MediaCodec
creation/initialization can fail for unknowable reasons at runtime (i.e. knowing
that it's supported isn't sufficient for ensuring the return value is non-null).

[pbos-webrtc, 2015-12-02 22:14:10]

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.

[noahric, 2015-12-02 22:19:34]

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 :)

[pbos-webrtc, 2015-12-02 22:26:38]

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?

[noahric, 2015-12-02 22:32:35]

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!