Closed Bug 1302935 Opened 3 years ago Closed 3 years ago

WebRTC VP9 support doesn't set the gof field correctly due to use of old libvpx

Categories

(Core :: WebRTC, defect, P1, major)

defect

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: jesup, Assigned: jesup)

Details

Attachments

(3 files)

When encoding and we have an encoded frame to pass to the Encoded() callback, we populate the codecSpecific structure.  

Because our libvpx is old, we have #ifdef LIBVPX_SVC's in the webrtc vp9 interface code (a number of SVC/etc related fields aren't defined in our libvpx).

Unfortunately, in VP9EncoderImpl::PopulateCodecSpecific(), the ifdef around "if (vp9_info->ss_data_available)" meant we didn't copy the 'gof' data into codecspecific (which is used in all but flexible mode).  Thus it was stack garbage, and if the num_frames_in_gof field happened to be much more than 0 it would trash stack memory and crash.
Rank: 10
Also enables vp9 in webrtc calls (as secondary codec)
Attachment #8791468 - Flags: review?(giles)
Attachment #8791468 - Flags: review?(giles) → review+
Pushed by rjesup@wgate.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd0093211582
enable vp9 in webrtc and fix missing gof fields in codecSpecific r=pkerr
Backout by rjesup@wgate.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/53c6a2ac40c7
Backed out changeset cd0093211582 for mda bustage (bad tests assume VP8 only)
VP9 turned out to be before VP8 since some change in JSEP; also the tests didn't like having more that one non-h264 codec in the lists.  And RegExps are a pain...
Attachment #8791776 - Flags: review?(drno)
Comment on attachment 8791776 [details] [diff] [review]
enable vp9 in webrtc and fix missing gof fields in codecSpecific

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

In general it looks okay, but I think the issues in sdpUtils.js and all.js are worth fixing (or answering) first.

::: dom/media/tests/mochitest/sdpUtils.js
@@ +27,4 @@
>  // Note, we don't bother removing the fmtp lines, which makes a good test
> +// for some SDP parsing issues.  It would be good to have a "removeAllButCodec(sdp, codec)" too
> +removeCodec: function(sdp, codec) {
> +    var updated_sdp = sdp.replace(new RegExp("a=rtpmap:" + codec + " VP8\\/90000\\r\\n",""),"");

How is this going to remove anything else then VP8 if it still searches for 'VP8\\/90000'?

::: dom/media/tests/mochitest/templates.js
@@ +511,3 @@
>    isnot(test.originalOffer.sdp.search("H264/90000"), -1, "H.264 should be present in the SDP offer");
> +    test.originalOffer.sdp = sdputils.removeCodec(sdputils.removeCodec(sdputils.removeCodec(
> +	test.originalOffer.sdp, 120), 121, 97));

Wouldn't it been easier to have removeCodec take an array as input? :)

::: modules/libpref/init/all.js
@@ +451,5 @@
>  pref("media.navigator.video.h264.level", 31); // 0x42E01f - level 3.1
>  pref("media.navigator.video.h264.max_br", 0);
>  pref("media.navigator.video.h264.max_mbps", 0);
>  pref("media.peerconnection.video.h264_enabled", false);
> +pref("media.peerconnection.video.vp9_enabled", true);

Is this intended, or a left over from testing?

I think line 442 contains the same pref (which is a different #ifdef block), but with a different default value?!
Attachment #8791776 - Flags: review?(drno) → review+
Pushed by rjesup@wgate.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/27f8a2467b31
enable vp9 in webrtc and fix missing gof fields in codecSpecific r=pkerr,drno
Also permatimeouts in test_peerConnection_scaleResolution.html, https://treeherder.mozilla.org/logviewer.html#?job_id=35961276&repo=mozilla-inbound
and yet more tests that need updating.  Note that signaling_unittests will not run for me currently - asserts in initialization
Attachment #8791863 - Flags: review?(drno)
Comment on attachment 8791863 [details] [diff] [review]
fix simulcast test and unittests for vp9

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

LGTM
Attachment #8791863 - Flags: review?(drno) → review+
Pushed by rjesup@wgate.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c0f5c81e90d8
enable vp9 in webrtc and fix missing gof fields in codecSpecific r=pkerr,drno
https://hg.mozilla.org/mozilla-central/rev/c0f5c81e90d8
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.