1022008 - Connect SDP negotiation to CodecSpecific structures for GMP codecs

Status: RESOLVED FIXED

(Core :: WebRTC: Signaling, defect)

Description

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

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.

Comment 1

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

Attachment: Support max-fs & max-fr in SDP for H.264; clean up video codec fmtp generation

Comment 2

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

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)

Comment 3

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

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.

Comment 4

[Ethan Hugg [:ehugg]]

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.

Comment 5

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

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

Comment 6

[Jason Smith [:jsmith]]

(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?

Comment 7

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

(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

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 9

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

Attachment: Hook up SDP negotiation for H.264 GMP codecs

Comment 10

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

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....

Comment 11

[Ethan Hugg [:ehugg]]

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?

Comment 12

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

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

Comment 13

[Carsten Book [:Tomcat]]

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

Comment 14

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/aaea56c38e45
https://hg.mozilla.org/releases/mozilla-aurora/rev/94714370dfc3