1328330 - vp8 error concealment should be removed

Status: RESOLVED FIXED

(Core :: WebRTC: Audio/Video, defect, P2)

Description

[johannkoenig]

Attachment: remove_vp8_ec.patch

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/56.0.2924.28 Safari/537.36

Steps to reproduce:

Searched source code for references to vp8 error concealment.


Actual results:

Found evidence of error concealment.


Expected results:

Error concealment in vp8 requires tracking many variables related to every frame processed while decoding in the off chance that a packet is dropped. This is massively expensive and causes slowdown when decoding even when the feature is not used.

The WebRTC implementation in Chromium does not use this feature and the implementation in Firefox does not use it either. It would be set via a flag passed to vpx_codec_dec_init, but looking at the code:
https://dxr.mozilla.org/mozilla-central/source/media/webrtc/trunk/webrtc/modules/video_coding/codecs/vp8/vp8_impl.cc?from=vp8_impl.cc

  vpx_codec_flags_t flags = 0;
#if !defined(WEBRTC_ARCH_ARM) && !defined(WEBRTC_ARCH_ARM64)
  flags = VPX_CODEC_USE_POSTPROC;
#ifdef INDEPENDENT_PARTITIONS
  flags |= VPX_CODEC_USE_INPUT_PARTITION;
#endif
#endif

  if (vpx_codec_dec_init(decoder_, vpx_codec_vp8_dx(), &cfg, flags)) {
    return WEBRTC_VIDEO_CODEC_MEMORY;
  }

the flag in question:
VPX_CODEC_USE_ERROR_CONCEALMENT is not set.

Found while attempting to simplify the libvpx configuration.

Comment 1

[johannkoenig]

I'm not sure what the next step is so I am not a good assignee.

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8823360 [details] [diff] [review]
remove_vp8_ec.patch

Review of attachment 8823360 [details] [diff] [review]:
-----------------------------------------------------------------

Per conversation with derf, we shouldn't need error concealment.  I do wonder if live-streamed sources might in theory use this (no recovery other than periodic iframes, and under the assumption it's better to show a mostly-correct set of frames until the next iframe than to freeze on every error -- which kinda depends on what you're trying todo.

Also one could use it for webrtc when the other side rejected all recovery mechanisms, or at least rejected NACK.  But probably not worth the cost.  And we're never turning the feature on -- though it's silly it has a perf impact on the decoder when it's not enabled....

NOTE FOR Sheriff: please edit the patch comment/summary before checkin.  We don't need general bug commentary in mercurial or lists of changed files.

r? ted (build peer)

Comment 3

[johannkoenig]

(In reply to Randell Jesup [:jesup] from comment #2)
> wonder if live-streamed sources might in theory use this (no recovery other
> than periodic iframes, and under the assumption it's better to show a
> mostly-correct set of frames until the next iframe than to freeze on every
> error -- which kinda depends on what you're trying todo.

Possibly. But keep in mind this particular implementation is only for vp8.
 
> Also one could use it for webrtc when the other side rejected all recovery
> mechanisms, or at least rejected NACK.  But probably not worth the cost. 
> And we're never turning the feature on -- though it's silly it has a perf
> impact on the decoder when it's not enabled....

The implementation could certainly be investigated and improved but this is currently dead code.

This patch conflicts with 1328744. I can rewrite it when that lands.

Comment 4

[johannkoenig]

Attachment: 0001-Bug-1328330-Remove-vp8-error-concealment.patch

Now that v1.6.1 is landed this is pretty trivial. All the changes under config/ are a result of re-running the script with '--enable-error-concealment' removed.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7898608a46f49335d17c897ee21217b799f538a8

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8827951 [details] [diff] [review]
0001-Bug-1328330-Remove-vp8-error-concealment.patch

Review of attachment 8827951 [details] [diff] [review]:
-----------------------------------------------------------------

r+
jya: I presume streaming use doesn't care about receiver-side error concealment support in libvpx?

Comment 6

[Jean-Yves Avenard [:jya]]

Comment on attachment 8827951 [details] [diff] [review]
0001-Bug-1328330-Remove-vp8-error-concealment.patch

Review of attachment 8827951 [details] [diff] [review]:
-----------------------------------------------------------------

It's possible to use libvpx to decode VP8 outside webrtc, but seeing that this isn't the default, it probably doesn't matter anyway

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bbbbcc7e8a1b

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/bbbbcc7e8a1b