Closed Bug 1112682 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: WebRTC: Signaling, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla37
Tracking Status
firefox35 --- wontfix
firefox36 --- wontfix
firefox37 --- fixed
b2g-v2.2 --- fixed

People

(Reporter: diego, Assigned: bwc)

References

Details

Attachments

(2 files, 1 obsolete file)

This started happening at m-c at around December 10th.
Hardware: x86 → ARM
Version: unspecified → Trunk
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.)
Blocks: 1091242
Component: WebRTC: Audio/Video → WebRTC: Signaling
Flags: needinfo?(docfaraday)
Seems plausible. I can try to look into it, but I do not have a device at present.
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?
Flags: needinfo?(docfaraday) → needinfo?(dwilson)
Flags: needinfo?(dwilson) → needinfo?(rjesup)
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.
So the Flame offers only VP8. Looks like we lost the ability to detect the H.264 hardware codec availability.
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?
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
Ugh, that macro isn't available there. Will try again in a few.
Ok, binaries with some logging are here:

http://ftp.mozilla.org/pub/mozilla.org/b2g/try-builds/bcampen@mozilla.com-6e4db50250b7/

If someone could reproduce and get the android logging, that should give me a better idea what is going on here.
The problem with the try builds is that they don't contain anything you can put on a real phone, only emulator builds :-(
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...
No "H264 hardware codec available" in the signaling logs...
Assignee: nobody → docfaraday
Status: NEW → ASSIGNED
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.
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
Severity: normal → major
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.
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?
Severity: major → normal
(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)
Flags: needinfo?(rjesup) → needinfo?(docfaraday)
(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?
Flags: needinfo?(docfaraday) → needinfo?(rjesup)
(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.
Flags: needinfo?(rjesup)
(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.
Attached patch Prefer hardware codecs. (obsolete) — Splinter Review
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
Attachment #8543354 - Flags: review?(rjesup)
Infra messed up that try. Here we go again.

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=2525de070143
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?
Fix prioritization.
Attachment #8543354 - Attachment is obsolete: true
Attachment #8543354 - Flags: review?(rjesup)
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
Attachment #8544248 - Flags: review?(rjesup)
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.
blocking-b2g: --- → 2.2?
Attachment #8544248 - Flags: review?(rjesup) → review+
Check back on try:

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=b0c892078fce
Flags: needinfo?(docfaraday)
https://hg.mozilla.org/mozilla-central/rev/e672c279ff31
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
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:
Attachment #8544248 - Flags: approval-mozilla-b2g37?
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
Attachment #8544248 - Flags: approval-mozilla-b2g37?
blocking-b2g: 2.2? → ---
No longer blocks: CAF-v3.0-FL-metabug
You need to log in before you can comment on or make changes to this bug.