vp8 error concealment should be removed

RESOLVED FIXED in Firefox 53

Status

()

Core
WebRTC: Audio/Video
P2
normal
Rank:
25
RESOLVED FIXED
4 months ago
3 months ago

People

(Reporter: johannkoenig, Assigned: johannkoenig)

Tracking

Trunk
mozilla53
Points:
---

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

4 months ago
Created attachment 8823360 [details] [diff] [review]
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.

Updated

4 months ago
Component: Untriaged → WebRTC: Audio/Video
Product: Firefox → Core
Assignee: nobody → johannkoenig
Status: UNCONFIRMED → ASSIGNED
Rank: 25
Ever confirmed: true
Priority: -- → P2
Attachment #8823360 - Flags: review?(rjesup)
(Assignee)

Comment 1

4 months ago
I'm not sure what the next step is so I am not a good assignee.
Assignee: johannkoenig → nobody
Status: ASSIGNED → NEW
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)
Attachment #8823360 - Flags: review?(ted)
Attachment #8823360 - Flags: review?(rjesup)
Attachment #8823360 - Flags: review+
(Assignee)

Comment 3

3 months ago
(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.
Assignee: nobody → johannkoenig
(Assignee)

Comment 4

3 months ago
Created attachment 8827951 [details] [diff] [review]
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
Attachment #8823360 - Attachment is obsolete: true
Attachment #8823360 - Flags: review?(ted)
Attachment #8827951 - Flags: review?(rjesup)
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?
Attachment #8827951 - Flags: review?(rjesup)
Attachment #8827951 - Flags: review?(jyavenard)
Attachment #8827951 - Flags: review+
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
Attachment #8827951 - Flags: review?(jyavenard) → review+
(Assignee)

Updated

3 months ago
Keywords: checkin-needed

Comment 7

3 months ago
bugherderlanding
https://hg.mozilla.org/integration/mozilla-inbound/rev/bbbbcc7e8a1b
Keywords: checkin-needed

Comment 8

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/bbbbcc7e8a1b
Status: NEW → RESOLVED
Last Resolved: 3 months ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.