Closed Bug 1146529 Opened 9 years ago Closed 9 years ago

preferred_codec ignored on answerer side

Categories

(Core :: WebRTC: Signaling, defect)

Product:
Core
Component:
WebRTC: Signaling
Version:
39 Branch
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla40
Tracking Flags:
Tracking Status
firefox40 --- fixed

People

(Reporter: drno, Assigned: bwc)

Details

Attachments

(2 files, 1 obsolete file)

MozReview Request: Bug 1146529 - Part 2: Rework part 1 to not rely on std::vector<UniquePtr>, since that is not supported by our buildconfig yet.
39 bytes, text/x-review-board-request
jesup
: review+
Details
MozReview Request: Bug 1146529: Allow codecs to be "strongly preferred", and set this bit on H264 hardware codecs. r=jesup
39 bytes, text/x-review-board-request
jesup
: review+
Details 
Was just in a Hello call with mreavy with todays Nightly 39.0a1 (2015-03-23) on Mac OSX.

On my side I have media.navigator.video.preferred_codec set to 126.

This resulted in me offering H264 as my first codec, which mreavy accepted on her end. On the other PeerConnection in her offer VP8 was offered first, before H264. But although I have the preferred_codec set my answer agreed on VP8.


First PeerConnection (I'm in the 10.0.1.x subnet):

Local SDP

v=0
o=mozilla...THIS_IS_SDPARTA-39.0a1 462934049296784117 0 IN IP4 0.0.0.0
s=-
t=0 0
a=sendrecv
a=fingerprint:sha-256 19:D5:CB:2C:78:10:31:3D:B3:67:C1:50:D9:29:B8:C2:69:E5:2D:C1:C1:A4:52:F3:5D:FE:C5:95:10:7C:06:0F
a=group:BUNDLE sdparta_0 sdparta_1
a=ice-options:trickle
a=msid-semantic:WMS *
m=audio 57593 RTP/SAVPF 109 9 0 8
c=IN IP4 72.251.228.4
a=candidate:0 1 UDP 2130379007 10.0.1.21 54297 typ host
a=candidate:0 2 UDP 2130379006 10.0.1.21 55585 typ host
a=candidate:1 1 UDP 1694236671 73.202.152.46 43037 typ srflx raddr 10.0.1.21 rport 54297
a=candidate:1 2 UDP 1694236670 73.202.152.46 36832 typ srflx raddr 10.0.1.21 rport 55585
a=candidate:3 1 UDP 100401151 72.251.228.4 57593 typ relay raddr 72.251.228.4 rport 57593
a=candidate:3 2 UDP 100401150 72.251.228.4 43837 typ relay raddr 72.251.228.4 rport 43837
a=candidate:4 1 UDP 16515071 72.251.228.4 16493 typ relay raddr 72.251.228.4 rport 16493
a=candidate:4 2 UDP 16515070 72.251.228.4 51993 typ relay raddr 72.251.228.4 rport 51993
a=sendrecv
a=end-of-candidates
a=extmap:1 urn:ietf:params:rtp-hdrext:ssrc-audio-level
a=ice-pwd:cb4ad53975be5c4f84e3f8df279015c2
a=ice-ufrag:8e0a72f5
a=mid:sdparta_0
a=msid:{3b4e8915-1e16-8c4b-bc33-219ffc4f2e3e} {0c9dc4ba-1be7-014f-af29-3072bf183f81}
a=rtcp-mux
a=rtpmap:109 opus/48000/2
a=rtpmap:9 G722/8000/1
a=rtpmap:0 PCMU/8000
a=rtpmap:8 PCMA/8000
a=setup:actpass
a=ssrc:3072588564 cname:{6805d395-5b32-004c-ba6a-d926e23af0f9}
m=video 50199 RTP/SAVPF 126 120 97
c=IN IP4 72.251.228.4
a=candidate:0 1 UDP 2130379007 10.0.1.21 55265 typ host
a=candidate:0 2 UDP 2130379006 10.0.1.21 54944 typ host
a=candidate:1 1 UDP 1694236671 73.202.152.46 48685 typ srflx raddr 10.0.1.21 rport 55265
a=candidate:1 2 UDP 1694236670 73.202.152.46 37998 typ srflx raddr 10.0.1.21 rport 54944
a=candidate:3 1 UDP 100401151 72.251.228.4 50199 typ relay raddr 72.251.228.4 rport 50199
a=candidate:4 1 UDP 16515071 72.251.228.4 27937 typ relay raddr 72.251.228.4 rport 27937
a=candidate:3 2 UDP 100401150 72.251.228.4 12682 typ relay raddr 72.251.228.4 rport 12682
a=candidate:4 2 UDP 16515070 72.251.228.4 23685 typ relay raddr 72.251.228.4 rport 23685
a=sendrecv
a=end-of-candidates
a=fmtp:126 profile-level-id=42e01f;level-asymmetry-allowed=1;packetization-mode=1
a=fmtp:120 max-fs=12288;max-fr=60
a=fmtp:97 profile-level-id=42e01f;level-asymmetry-allowed=1
a=ice-pwd:cb4ad53975be5c4f84e3f8df279015c2
a=ice-ufrag:8e0a72f5
a=mid:sdparta_1
a=msid:{3b4e8915-1e16-8c4b-bc33-219ffc4f2e3e} {498779b6-3ac4-3a4f-9992-5b063aead62c}
a=rtcp-fb:126 nack
a=rtcp-fb:126 nack pli
a=rtcp-fb:126 ccm fir
a=rtcp-fb:120 nack
a=rtcp-fb:120 nack pli
a=rtcp-fb:120 ccm fir
a=rtcp-fb:97 nack
a=rtcp-fb:97 nack pli
a=rtcp-fb:97 ccm fir
a=rtcp-mux
a=rtpmap:126 H264/90000
a=rtpmap:120 VP8/90000
a=rtpmap:97 H264/90000
a=setup:actpass
a=ssrc:2568818906 cname:{6805d395-5b32-004c-ba6a-d926e23af0f9}

Remote SDP

v=0
o=mozilla...THIS_IS_SDPARTA-39.0a1 4067110179145963616 0 IN IP4 0.0.0.0
s=-
t=0 0
a=sendrecv
a=fingerprint:sha-256 9E:8C:CF:27:A4:DA:F7:EB:9D:4B:D2:51:84:74:DD:28:00:CF:67:FD:3E:49:BA:E7:B5:3D:25:56:54:0B:0F:3F
a=group:BUNDLE sdparta_0 sdparta_1
a=ice-options:trickle
a=msid-semantic:WMS *
m=audio 9 RTP/SAVPF 109
c=IN IP4 0.0.0.0
a=candidate:0 1 UDP 2128609535 192.168.1.19 54168 typ host
a=candidate:1 1 UDP 1692467199 173.49.141.196 54168 typ srflx raddr 192.168.1.19 rport 54168
a=candidate:3 1 UDP 98631679 72.251.228.4 27316 typ relay raddr 72.251.228.4 rport 27316
a=candidate:4 1 UDP 14745599 72.251.228.4 64473 typ relay raddr 72.251.228.4 rport 64473
a=recvonly
a=extmap:1 urn:ietf:params:rtp-hdrext:ssrc-audio-level
a=ice-pwd:3eb245934a06f885f0d761970a4028a7
a=ice-ufrag:718bfe2d
a=mid:sdparta_0
a=rtcp-mux
a=rtpmap:109 opus/48000/2
a=setup:active
m=video 9 RTP/SAVPF 126
c=IN IP4 0.0.0.0
a=recvonly
a=fmtp:126 profile-level-id=42e01f;level-asymmetry-allowed=1;packetization-mode=1
a=ice-pwd:3eb245934a06f885f0d761970a4028a7
a=ice-ufrag:718bfe2d
a=mid:sdparta_1
a=rtcp-fb:126 nack
a=rtcp-fb:126 nack pli
a=rtcp-fb:126 ccm fir
a=rtcp-mux
a=rtpmap:126 H264/90000
a=setup:active

Second PeerConnection:

Local SDP

v=0
o=mozilla...THIS_IS_SDPARTA-39.0a1 5011930120336263692 0 IN IP4 0.0.0.0
s=-
t=0 0
a=sendrecv
a=fingerprint:sha-256 D3:97:49:E3:B8:00:D1:D6:22:E6:DA:25:1D:5A:8A:DD:50:8C:DB:67:A2:6B:50:49:38:75:0D:D5:D5:CC:83:13
a=group:BUNDLE sdparta_0 sdparta_1
a=ice-options:trickle
a=msid-semantic:WMS *
m=audio 11214 RTP/SAVPF 109
c=IN IP4 72.251.228.4
a=candidate:0 1 UDP 2130379007 10.0.1.21 57114 typ host
a=candidate:1 1 UDP 1694236671 73.202.152.46 49000 typ srflx raddr 10.0.1.21 rport 57114
a=candidate:3 1 UDP 100401151 72.251.228.4 11214 typ relay raddr 72.251.228.4 rport 11214
a=candidate:4 1 UDP 16515071 72.251.228.4 13228 typ relay raddr 72.251.228.4 rport 13228
a=recvonly
a=end-of-candidates
a=extmap:1 urn:ietf:params:rtp-hdrext:ssrc-audio-level
a=ice-pwd:5484f7ed81d5731bc7ddcad16b70144d
a=ice-ufrag:045d3a02
a=mid:sdparta_0
a=rtcp-mux
a=rtpmap:109 opus/48000/2
a=setup:active
m=video 11214 RTP/SAVPF 120
c=IN IP4 72.251.228.4
a=recvonly
a=fmtp:120 max-fs=12288;max-fr=60
a=ice-pwd:5484f7ed81d5731bc7ddcad16b70144d
a=ice-ufrag:045d3a02
a=mid:sdparta_1
a=rtcp-fb:120 nack
a=rtcp-fb:120 nack pli
a=rtcp-fb:120 ccm fir
a=rtcp-mux
a=rtpmap:120 VP8/90000
a=setup:active

Remote SDP

v=0
o=mozilla...THIS_IS_SDPARTA-39.0a1 6372405626218455883 0 IN IP4 0.0.0.0
s=-
t=0 0
a=sendrecv
a=fingerprint:sha-256 F4:9D:F6:39:AE:D1:6C:1F:BB:A4:31:08:28:CA:B3:5E:BC:06:26:DA:05:F4:1B:7C:B5:38:3C:5A:BC:94:79:B3
a=group:BUNDLE sdparta_0 sdparta_1
a=ice-options:trickle
a=msid-semantic:WMS *
m=audio 9 RTP/SAVPF 109 9 0 8
c=IN IP4 0.0.0.0
a=candidate:0 1 UDP 2128609535 192.168.1.19 50794 typ host
a=candidate:0 2 UDP 2128609534 192.168.1.19 50795 typ host
a=candidate:1 1 UDP 1692467199 173.49.141.196 50794 typ srflx raddr 192.168.1.19 rport 50794
a=candidate:3 1 UDP 98631679 72.251.228.4 63304 typ relay raddr 72.251.228.4 rport 63304
a=candidate:4 1 UDP 14745599 72.251.228.4 34584 typ relay raddr 72.251.228.4 rport 34584
a=candidate:1 2 UDP 1692467198 173.49.141.196 50795 typ srflx raddr 192.168.1.19 rport 50795
a=candidate:3 2 UDP 98631678 72.251.228.4 17558 typ relay raddr 72.251.228.4 rport 17558
a=candidate:4 2 UDP 14745598 72.251.228.4 53412 typ relay raddr 72.251.228.4 rport 53412
a=sendrecv
a=extmap:1 urn:ietf:params:rtp-hdrext:ssrc-audio-level
a=ice-pwd:74081a6ac4836933268cf48173b694c1
a=ice-ufrag:f6d84f85
a=mid:sdparta_0
a=msid:{5686b92b-edcc-4b3e-81eb-cbcfdb7907ce} {4df29d66-38ff-46b3-86ef-7984c6803a79}
a=rtcp-mux
a=rtpmap:109 opus/48000/2
a=rtpmap:9 G722/8000/1
a=rtpmap:0 PCMU/8000
a=rtpmap:8 PCMA/8000
a=setup:actpass
a=ssrc:671141787 cname:{859acac0-873e-4f9b-85de-b8656b5bff42}
m=video 9 RTP/SAVPF 120 126 97
c=IN IP4 0.0.0.0
a=candidate:0 1 UDP 2128609535 192.168.1.19 50796 typ host
a=candidate:0 2 UDP 2128609534 192.168.1.19 50797 typ host
a=candidate:1 1 UDP 1692467199 173.49.141.196 50796 typ srflx raddr 192.168.1.19 rport 50796
a=candidate:3 1 UDP 98631679 72.251.228.4 41193 typ relay raddr 72.251.228.4 rport 41193
a=candidate:1 2 UDP 1692467198 173.49.141.196 50797 typ srflx raddr 192.168.1.19 rport 50797
a=candidate:4 1 UDP 14745599 72.251.228.4 10703 typ relay raddr 72.251.228.4 rport 10703
a=sendrecv
a=fmtp:120 max-fs=12288;max-fr=60
a=fmtp:126 profile-level-id=42e01f;level-asymmetry-allowed=1;packetization-mode=1
a=fmtp:97 profile-level-id=42e01f;level-asymmetry-allowed=1
a=ice-pwd:74081a6ac4836933268cf48173b694c1
a=ice-ufrag:f6d84f85
a=mid:sdparta_1
a=msid:{5686b92b-edcc-4b3e-81eb-cbcfdb7907ce} {5ee99d91-7921-4dab-95dd-96207a5aeed0}
a=rtcp-fb:120 nack
a=rtcp-fb:120 nack pli
a=rtcp-fb:120 ccm fir
a=rtcp-fb:126 nack
a=rtcp-fb:126 nack pli
a=rtcp-fb:126 ccm fir
a=rtcp-fb:97 nack
a=rtcp-fb:97 nack pli
a=rtcp-fb:97 ccm fir
a=rtcp-mux
a=rtpmap:120 VP8/90000
a=rtpmap:126 H264/90000
a=rtpmap:97 H264/90000
a=setup:actpass
a=ssrc:1144501296 cname:{859acac0-873e-4f9b-85de-b8656b5bff42}
This is expected; codec prioritization is based on the priority in the offer. We do not try to do anything fancy like picking a codec that both the offerer and ourselves "like".
(In reply to Byron Campen [:bwc] from comment #1)
> This is expected; codec prioritization is based on the priority in the
> offer. We do not try to do anything fancy like picking a codec that both the
> offerer and ourselves "like".

Ok. But how does a FxOS device as the answerer then chose H264? In case the device has H264 hardware support it has a "strong preference" for doing H264 over VP8 I think.
I agree with Nils on this one. Generally if the answerer strongly prefers one or
the other codec (say because it has hardware) it needs to be able to reflect
that in the negotiation.
Ok, so there will need to be some work on the prioritization algorithm. This sort of thing will behave inconsistently if both endpoints have a different "strongly preferred" codec, of course.
The previous code (pre-rewrite) did implement prioritization for H264 as an answerer when a HW codec was available.  Note that this is separate from the "preferred_codec" var which is meant solely for testing.  I believe however that the "preferred_codec" var also overrode the offerer's preference IIRC.

Basically, in SDP, the offerer provides it's preference list, but the answerer gets to decide within that list, and can pick a lower-preference one, or multiple.

We should make sure that B2G 37 (2.2) is still selecting the HW codec when appropriate as an answerer as well.
Assignee: nobody → docfaraday
Attached file MozReview Request: bz://1146529/bwc (obsolete) — 
/r/6107 - Bug 1146529: Allow codecs to be "strongly preferred", and set this bit on H264 hardware codecs.

Pull down this commit:

hg pull review -r 5aeeaaa268e826145cb417b68347a4cbd30ee60c
Comment on attachment 8583380 [details]
MozReview Request: bz://1146529/bwc

Did a little refactoring too.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=da62f8071efc
Attachment #8583380 - Flags: review?(rjesup)
Comment on attachment 8583380 [details]
MozReview Request: bz://1146529/bwc

Actually, this requires a non-trivial rebase, let me do that first.
Attachment #8583380 - Flags: review?(rjesup)
Comment on attachment 8583380 [details]
MozReview Request: bz://1146529/bwc

/r/6107 - Bug 1146529: Allow codecs to be "strongly preferred", and set this bit on H264 hardware codecs.

Pull down this commit:

hg pull review -r a7c09b93117234029934b536ff6de9972f4a3410
Comment on attachment 8583380 [details]
MozReview Request: bz://1146529/bwc

Rebased, and applied some additional cleanup for some mess I tripped over. Cleanup was easiest if I could use vector<UniquePtr>, so I guess we'll see if that limitation has been fixed yet. If not, I can adjust.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4b8d8eff59d6
Attachment #8583380 - Flags: review?(rjesup)
Attachment #8583380 - Flags: review?(rjesup) → review+
Comment on attachment 8583380 [details]
MozReview Request: bz://1146529/bwc

/r/6107 - Bug 1146529: Allow codecs to be "strongly preferred", and set this bit on H264 hardware codecs. r=jesup
/r/6219 - Bug 1146529 - Part 2: Rework part 1 to not rely on std::vector<UniquePtr>, since that is not supported by our buildconfig yet.

Pull down these commits:

hg pull review -r 6ca13eb8115fab6ad0fc9387ad87ab30f9d3553c
Attachment #8583380 - Flags: review+
Comment on attachment 8583380 [details]
MozReview Request: bz://1146529/bwc

Some non-trivial cleanup in part 2 that needs review.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a66d0ff22007
Attachment #8583380 - Flags: review?(rjesup)
Comment on attachment 8583380 [details]
MozReview Request: bz://1146529/bwc

https://reviewboard.mozilla.org/r/6105/#review5247

::: media/webrtc/signaling/src/sdp/SipccSdpAttributeList.h
(Diff revision 3)
> -  SdpAttribute* mAttributes[kNumAttributeTypes];
> +  PtrVector<SdpAttribute> mAttributes;

I don't see that we really gain anything from making this statically-sized array into a PtrVector-that-is-statically-sized.  It saves needing a destructor that deletes them is about the limit of it, and it adds needing to resize it (and allocate) in the constructor.

And nsAutoTArray<UniquePtr<SdpAttribute>> would avoid the dynamic-resize/separate-allocation.

I'd leave this one a static array unless you plan to really make it dynamic soon.  But I'll leave it up to you.
Attachment #8583380 - Flags: review?(rjesup)
https://hg.mozilla.org/mozilla-central/rev/de811f56ae40
https://hg.mozilla.org/mozilla-central/rev/acfd5ce98b6d
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40

        Attachment #8583380 -
        Attachment is obsolete: true

        Attachment #8619849 -
        Flags: review+

        Attachment #8619850 -
        Flags: review+

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: