Closed Bug 1022008 Opened 6 years ago Closed 6 years ago

Connect SDP negotiation to CodecSpecific structures for GMP codecs

Categories

(Core :: WebRTC: Signaling, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33
blocking-b2g 2.0+
Tracking Status
firefox31 --- wontfix
firefox32 --- fixed
firefox33 --- fixed
b2g-v2.0 --- fixed
b2g-v2.1 --- fixed

People

(Reporter: jesup, Assigned: jesup)

References

(Blocks 1 open bug)

Details

(Whiteboard: [ft:loop])

Attachments

(2 files)

Right now H264 SDP negotiation isn't hooked into the codecspecific structures.  We need to do that (for GMP and for OMX H.264 codecs) and we need to copy the data from the internal structures to the GMP versions of them. These are marked in the WebrtcGMPVideoCodec.cpp file.
Blocks: OpenH264
Assignee: nobody → rjesup
Comment on attachment 8447558 [details] [diff] [review]
Support max-fs & max-fr in SDP for H.264; clean up video codec fmtp generation

This is the first part of the SDP cleanup for H.264 (OpenH264 and OMX HW H.264)
Attachment #8447558 - Flags: review?(ethanhugg)
This is needed for 2.0 to ensure OpenH264 (etc) doesn't try to send us larger resolutions than we can handle in an interactive call.
blocking-b2g: --- → 2.0?
Whiteboard: [leave-open][webrtc-uplift]
Comment on attachment 8447558 [details] [diff] [review]
Support max-fs & max-fr in SDP for H.264; clean up video codec fmtp generation

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

Looks good to me.  I tried it out a bit on OSX with a working version of the plugin.

::: media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c
@@ +1188,5 @@
> +            if (media_type == RTP_H264_P1) {
> +                (void) sdp_attr_set_fmtp_pack_mode(sdp_p, level, 0, a_inst,
> +                                                   1);
> +            }
> +        //(void) sdp_attr_set_fmtp_max_mbps(sdp_p, level, 0, a_inst, max_mbps);

I know this comment section is from the old code, but you may want to either remove this list of decls or explain the TODO.
Attachment #8447558 - Flags: review?(ethanhugg) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/b5b7809e8272
We'll want to uplift this to Aurora so we can ensure OpenH264 desktop don't try to send us more than we can handle
(In reply to Randell Jesup [:jesup] from comment #3)
> This is needed for 2.0 to ensure OpenH264 (etc) doesn't try to send us
> larger resolutions than we can handle in an interactive call.

I'm confused - I thought the 2.0 H264 WebRTC implementation on Firefox OS was going to be directly making use of hardware decoder, not the OpenH264 implementation. Can you clarify?
Flags: needinfo?(rjesup)
(In reply to Jason Smith [:jsmith] from comment #6)
> (In reply to Randell Jesup [:jesup] from comment #3)
> > This is needed for 2.0 to ensure OpenH264 (etc) doesn't try to send us
> > larger resolutions than we can handle in an interactive call.
> 
> I'm confused - I thought the 2.0 H264 WebRTC implementation on Firefox OS
> was going to be directly making use of hardware decoder, not the OpenH264
> implementation. Can you clarify?

No, but I need to ensure I advertise that OMX H.264 can only support up to HVGA, so an OpenH264 desktop we talk to doesn't try to send us VGA (or HD).  That means I need to say "max framesize I support receiving is X", which is what that patch is
Flags: needinfo?(rjesup)
blocking-b2g: 2.0? → 2.0+
Comment on attachment 8452139 [details] [diff] [review]
Hook up SDP negotiation for H.264 GMP codecs

Turns out vcmCheckAttribs assumed there was only one video codec....
Attachment #8452139 - Flags: review?(ethanhugg)
Whiteboard: [leave-open][webrtc-uplift] → [leave-open][webrtc-uplift]ft:loop
Comment on attachment 8452139 [details] [diff] [review]
Hook up SDP negotiation for H.264 GMP codecs

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

::: media/webrtc/signaling/src/media-conduit/CodecConfig.h
@@ +108,5 @@
> +    mPacketizationMode(1)
> +  {
> +    if (h264) {
> +      if (max_fs == 0 || (unsigned int) h264->max_fs < max_fs) {
> +        mMaxFrameSize = h264->max_fs;

So, I see the max_fs zero case handled, but if h264->max_fs is zero wouldn't we want to keep mMaxFrameSize equal to max_fs?
Attachment #8452139 - Flags: review?(ethanhugg) → review+
Component: Video/Audio → WebRTC: Signaling
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [leave-open][webrtc-uplift]ft:loop → [webrtc-uplift]ft:loop
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.