1112682 - B2G WebRTC falls back to VP8 when H.264 is enabled

Status: RESOLVED FIXED

(Core :: WebRTC: Signaling, defect)

Description

[Diego Wilson [:diego]]

This started happening at m-c at around December 10th.

Comment 1

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

This was very likely fallout from the negotiation rewrite (signaling) done in bug 1091242.  Byron?

(it can be tricky to make sure the codec reservation rules are followed (and any temporary grab is released before grabbing it again, etc), and if not we could think it's in-use when it isn't.  Or the landing simply broke HW codec usage.)

Comment 2

[Byron Campen [:bwc]]

Seems plausible. I can try to look into it, but I do not have a device at present.

Comment 3

[Byron Campen [:bwc]]

A couple of questions:

When the latest m-c code is the offerer, is H264 of any kind offered? If not, can I get nspr signaling:6 logging? (I'm interested in whether the string "H264 hardware codec available" appears)?

When the latest m-c code is the answerer, and release (or some other version prior to the 10th) is the offerer, what is in the offer/answer?

Comment 4

[Nils Ohlmeier [:drno]]

I'm able to reproduce it with 20141219040202 flame build and Nightly (37.0a1 (2014-12-19)) on apprtc.
Desktop offered 120, 126, 97 as expected.
Flame chose 120 = VP8 out of that list.

Comment 5

[Nils Ohlmeier [:drno]]

So the Flame offers only VP8. Looks like we lost the ability to detect the H.264 hardware codec availability.

Comment 6

[Byron Campen [:bwc]]

So this is the beginning of the block of code that configures out codec parameters, and disables codecs where necessary:

http://dxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp#797

I've looked over this a few times, and am not spotting any problems. Is it possible that something else is holding a reservation at this point (CreateOffer)? Or is there something that could be causing the prefs fetch to fail? Or were the reservations always failing before, and some other bug was masking the failure?

Comment 7

[Byron Campen [:bwc]]

I've done a try push with some temporary debug logging that might give us an idea of what is happening (the logging uses __android_log_print(ANDROID_LOG_ERROR, "OMXCodecWrapper", ...) so it should show up without any trouble?).

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=f5f05a76d919

Comment 8

[Byron Campen [:bwc]]

Ugh, that macro isn't available there. Will try again in a few.

Comment 9

[Byron Campen [:bwc]]

Ok, binaries with some logging are here:

http://ftp.mozilla.org/pub/mozilla.org/b2g/try-builds/[e-mail]-6e4db50250b7/

If someone could reproduce and get the android logging, that should give me a better idea what is going on here.

Comment 10

[Nils Ohlmeier [:drno]]

The problem with the try builds is that they don't contain anything you can put on a real phone, only emulator builds :-(

Comment 11

[Byron Campen [:bwc]]

Surely there is some way to get our build farm to build a custom b2g binary without requiring a push to inbound or a project branch...

Comment 12

[Nils Ohlmeier [:drno]]

No "H264 hardware codec available" in the signaling logs...

Comment 13

[Byron Campen [:bwc]]

Attachment: (NOT FOR CHECKIN) Some debug that might prove helpful.

Comment 14

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

right - that's what I expected; the codec reservation stuff is touchy, since you can only have one instance of encoder/decoder open at a time.

You need to look where it was allocated and freed.

Comment 15

[Nils Ohlmeier [:drno]]

According to my logging this condition is not/no longer (?) true:
http://dxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp#813

Comment 16

[Nils Ohlmeier [:drno]]

So after adding this:

pref('media.peerconnection.video.h264_enabled', true);

to this file:

/system/b2g/defaults/pref/user.js

I can get the Flame to offer H264, but only as second codec after VP8.

Adding this setting:

pref('media.navigator.video.preferred_codec', 126);

To the same file no longer seem to have to desired effect. Neither gets 126 offered before 120, nor does 126 picked as the preferred code if it is offered.

Comment 17

[Nils Ohlmeier [:drno]]

Sorry did not mean to set the priority.

Randel: should enabling H264 and setting it as preferred code be part of our default builds, or is this something the vendors still have to activate in their builds?

Comment 18

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

(In reply to Nils Ohlmeier [:drno] from comment #17)
> Sorry did not mean to set the priority.
> 
> Randel: should enabling H264 and setting it as preferred code be part of our
> default builds, or is this something the vendors still have to activate in
> their builds?

It's vendor activation - we only support HW H.264 on specific devices (chipsets+firmware) that have been tested and verified.  For example, until DSP code Flame 1530.x (IIRC) you couldn't modify the bitrate inbetween iframes.  Earlier versions had a 2-second decode delay.   So we can't reasonably enable-by-default.  Even for Flames, you have to have an up-to-date base build.

(to earlier comment)
Ok, I would have added it to /data/b2g/mozilla/<profile>/prefs.js, but that probably works too.
Appearing second is wrong.  That sounds like a regression from the sdparta rewrite.  HW codecs, if enabled, are always supposed to have higher priority, then built-in, then GMP (OpenH264 - not on B2G)

Comment 19

[Byron Campen [:bwc]]

(In reply to Randell Jesup [:jesup] from comment #18)
> (In reply to Nils Ohlmeier [:drno] from comment #17)
> > Sorry did not mean to set the priority.
> > 
> > Randel: should enabling H264 and setting it as preferred code be part of our
> > default builds, or is this something the vendors still have to activate in
> > their builds?
> 
> It's vendor activation - we only support HW H.264 on specific devices
> (chipsets+firmware) that have been tested and verified.  For example, until
> DSP code Flame 1530.x (IIRC) you couldn't modify the bitrate inbetween
> iframes.  Earlier versions had a 2-second decode delay.   So we can't
> reasonably enable-by-default.  Even for Flames, you have to have an
> up-to-date base build.
> 
> (to earlier comment)
> Ok, I would have added it to /data/b2g/mozilla/<profile>/prefs.js, but that
> probably works too.
> Appearing second is wrong.  That sounds like a regression from the sdparta
> rewrite.  HW codecs, if enabled, are always supposed to have higher
> priority, then built-in, then GMP (OpenH264 - not on B2G)

   Ah, I did not know that we were preferring hardware codecs. I can fix that. So the calculation should prefer first the preferred codec set in prefs, then hardware codecs, then everything else?

   Is the preference what this bug is really about? Do we actually have any evidence that h264 is not offered if it is enabled in prefs?

Comment 20

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

(In reply to Byron Campen [:bwc] from comment #19)

>    Ah, I did not know that we were preferring hardware codecs. I can fix
> that. So the calculation should prefer first the preferred codec set in
> prefs, then hardware codecs, then everything else?

Yes, that should have been documented in VcmSIPCCBindings.cpp.  See the old code and compare.

>    Is the preference what this bug is really about? Do we actually have any
> evidence that h264 is not offered if it is enabled in prefs?

It appears Nils' tests above show that it's offered (but second).  Also his test shows "preferred_codec" got broken as well.

Comment 21

[Byron Campen [:bwc]]

(In reply to Randell Jesup [:jesup] from comment #20)
> (In reply to Byron Campen [:bwc] from comment #19)
> 
> >    Is the preference what this bug is really about? Do we actually have any
> > evidence that h264 is not offered if it is enabled in prefs?
> 
> It appears Nils' tests above show that it's offered (but second).  Also his
> test shows "preferred_codec" got broken as well.

   I've tested preferrec_codec myself and it worked. Not sure what the deal was there.

Comment 22

[Byron Campen [:bwc]]

Attachment: Prefer hardware codecs.

Comment 23

[Byron Campen [:bwc]]

Comment on attachment 8543354 [details] [diff] [review]
Prefer hardware codecs.

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

try:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=24b827070fc7

Comment 24

[Byron Campen [:bwc]]

Infra messed up that try. Here we go again.

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=2525de070143

Comment 25

[Nils Ohlmeier [:drno]]

With the latest patch and the setting:

pref('media.peerconnection.video.h264_enabled', true);

I still get an offer like this:

m=video 9 RTP/SAVPF 120 126

If I'm not mistaken I should not have to use the preferred_codec setting, correct?

Comment 26

[Byron Campen [:bwc]]

Attachment: Prefer hardware codecs

Fix prioritization.

Comment 27

[Byron Campen [:bwc]]

Comment on attachment 8544248 [details] [diff] [review]
Prefer hardware codecs

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

try:

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=054f7d4a8a49

Comment 28

[Nils Ohlmeier [:drno]]

attachment 8544248 [details] [diff] [review] seems to do what we want it: with pref('media.peerconnection.video.h264_enabled', true); set my flame offered:

m=video 9 RTP/SAVPF 126 120
c=IN IP4 0.0.0.0
a=sendrecv
a=fmtp:126 PROFILE=0;LEVEL=0;profile-level-id=42e00c;packetization-mode=1;level-asymmetry-allowed=1;max-br=11880;parameter-add=1;usedtx=0;stereo=0;useinbandfec=0;cbr=0
a=fmtp:120 PROFILE=0;LEVEL=0;packetization-mode=0;level-asymmetry-allowed=1;max-fs=1200;max-fr=30;parameter-add=1;usedtx=0;stereo=0;useinbandfec=0;cbr=0

And the desktop picked H264 from that list.

Comment 29

[Byron Campen [:bwc]]

Check back on try:

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=b0c892078fce

Comment 30

[Byron Campen [:bwc]]

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

Comment 31

[Carsten Book [:Tomcat]]

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

Comment 32

[Michael Vines [:m1] [:evilmachines]]

Comment on attachment 8544248 [details] [diff] [review]
Prefer hardware codecs

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: 
Testing completed: 
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch:

Comment 33

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8544248 [details] [diff] [review]
Prefer hardware codecs

Per the target milestone flag, this landed on mozilla-central prior to b2g37 being branched. This changeset is already there.
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/e672c279ff31