Closed Bug 1002306 Opened 10 years ago Closed 10 years ago

WebRTC VP8 doesn't work from b2g to desktop.

Categories

(Core :: WebRTC, defect)

Other
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla32
Tracking Status
firefox30 --- unaffected
firefox31 --- fixed
firefox32 --- fixed

People

(Reporter: ayang, Assigned: jesup)

Details

(Keywords: verifyme)

Attachments

(1 file, 1 obsolete file)

OS: Mac OS X → Gonk (Firefox OS)
Hardware: x86 → Other
Comment on attachment 8413793 [details] [diff] [review]
don't accidentally disable non-H264 codecs in the OMX code

Note: fixed by code inspection; I haven't tested this.
Attachment #8413793 - Flags: review?(ekr)
Comment on attachment 8413793 [details] [diff] [review]
don't accidentally disable non-H264 codecs in the OMX code

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

::: media/webrtc/signaling/src/media/VcmSIPCCBinding.cpp
@@ +2126,5 @@
>    // and will reject registration of those not in it.
>    // TODO: bug 995884 to support H.264 in WebRTC.org code.
>    if (config->mName != "I420") {
>      // Do nothing for non-I420 config.
> +    return 0;

This doesn't seem right. The idea of the more general code is to only add codecs to the SDP that we have support for. So if someone adds "VP9", we don't want to add that if nobody has installed a plugin (the I420 thing is a temporary).

So, this should return errors if you ask for an unknown codec. In this case it should:

- Shortcut return for VP8
- Create the OMX encoder for H.264 ("I420")
- Otherwise return an error.
Attachment #8413793 - Flags: review?(ekr) → review-
Attachment #8413793 - Attachment is obsolete: true
Comment on attachment 8413849 [details] [diff] [review]
don't accidentally disable non-H264 codecs in the OMX code

Addresses comments; this patch does more than just fix the mistake in the original patch
Attachment #8413849 - Flags: review?(ekr)
Assignee: nobody → rjesup
Whiteboard: [webrtc-uplift]
Comment on attachment 8413849 [details] [diff] [review]
don't accidentally disable non-H264 codecs in the OMX code

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

lgtm
Attachment #8413849 - Flags: review?(ekr) → review+
Comment on attachment 8413849 [details] [diff] [review]
don't accidentally disable non-H264 codecs in the OMX code

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 911046

User impact if declined: video webrtc calls won't work

Testing completed (on m-c, etc.): Just landed on inbound; local testing done

Risk to taking this patch (and alternatives if risky): Virtually no risk.  Obviously broken code; patch makes things much cleaner.

String or IDL/UUID changes made by this patch: none
Attachment #8413849 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/32c92d57273a
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Attachment #8413849 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Hi Jason, is this something we can help with if it needs QA work from Desktop? Let me or Anthony know. Thanks!
Flags: needinfo?(jsmith)
Keywords: verifyme
(In reply to Liz Henry :lizzard from comment #11)
> Hi Jason, is this something we can help with if it needs QA work from
> Desktop? Let me or Anthony know. Thanks!

Yes, that should be possible so long as you guys have a Flame device to test with. I think Marc shipped one to Anthony to my understanding.
Flags: needinfo?(jsmith)
(In reply to Jason Smith [:jsmith] from comment #12)
> Yes, that should be possible so long as you guys have a Flame device to test
> with. I think Marc shipped one to Anthony to my understanding.

Shipped last week, still waiting for it to arrive though.
Whiteboard: [webrtc-uplift]
You need to log in before you can comment on or make changes to this bug.