1438289 - Implement RsdparsaSdpMediaSection::AddCodec and RsdparsaSdpMediaSection::ClearCodecs

Status: RESOLVED FIXED

(Core :: WebRTC: Signaling, enhancement, P3)

Comment 1

[Johannes Willbold]

Attachment: Bug 1438289: Implemented RsdparsaSdpMediaSection::AddCodec and ::ClearCodecs.

Implemented RsdparsaSdpMediaSection::ClearCodecs and RsdparsaSdpMediaSection::AddCodec
Added sdp_media_clear_codecs
Added corresponding C++/Rust glue code
Added rust tratir t convert StringView to rust std::String
Added testcase CheckAddCodec and CheckClearCodec

Review commit: https://reviewboard.mozilla.org/r/249786/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/249786/

Comment 2

[Dan Minor [:dminor]]

Comment on attachment 8983932 [details]
Bug 1438289: Implemented RsdparsaSdpMediaSection::AddCodec and ::ClearCodecs.

https://reviewboard.mozilla.org/r/249786/#review256168

::: commit-message-f01bb:6
(Diff revision 1)
> +Bug 1438289: Implemented RsdparsaSdpMediaSection::AddCodec and ::ClearCodecs. r=dminor
> +
> +Implemented RsdparsaSdpMediaSection::ClearCodecs and RsdparsaSdpMediaSection::AddCodec
> +Added sdp_media_clear_codecs
> +Added corresponding C++/Rust glue code
> +Added rust tratir t convert StringView to rust std::String

typo: trait to ?

::: media/webrtc/signaling/src/sdp/RsdparsaSdpMediaSection.cpp:139
(Diff revision 1)
>  }
>  
>  void
>  RsdparsaSdpMediaSection::AddCodec(const std::string& pt,
>                                    const std::string& name,
>                                    uint32_t clockrate, uint16_t channels)

Please check with Byron as to whether channels should be uint16_t or uint32_t. I did a quick search of the code and it looks like uint32_t is used by the current sdp and jsep code, so maybe AddCodec is incorrect.

::: media/webrtc/signaling/src/sdp/rsdparsa_capi/src/media_section.rs:168
(Diff revision 1)
> +                                            return NS_ERROR_INVALID_ARG;
> +                                        }
> +                                    },
> +                                    frequency: clockrate,
> +                                    channels: Some(channels as u32),
> +                                };

nit: check indent here

Comment 3

[Johannes Willbold]

Comment on attachment 8983932 [details]
Bug 1438289: Implemented RsdparsaSdpMediaSection::AddCodec and ::ClearCodecs.

https://reviewboard.mozilla.org/r/249786/#review256168

> typo: trait to ?

yes

> Please check with Byron as to whether channels should be uint16_t or uint32_t. I did a quick search of the code and it looks like uint32_t is used by the current sdp and jsep code, so maybe AddCodec is incorrect.

Qoute from bwc: "Even uint16_t is overkill, to be honest. It should be fine."

Comment 4

[Johannes Willbold]

Comment on attachment 8983932 [details]
Bug 1438289: Implemented RsdparsaSdpMediaSection::AddCodec and ::ClearCodecs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/249786/diff/1-2/

Comment 5

[Johannes Willbold]

Comment on attachment 8983932 [details]
Bug 1438289: Implemented RsdparsaSdpMediaSection::AddCodec and ::ClearCodecs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/249786/diff/2-3/

Comment 6

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 5e5ed3c7fecf61699f2daabe8f6ab4068a6cc0db -d 7deab88709c7: rebasing 467554:5e5ed3c7fecf "Bug 1438289: Implemented RsdparsaSdpMediaSection::AddCodec and ::ClearCodecs. r=dminor" (tip)
merging media/webrtc/signaling/gtest/sdp_unittests.cpp
merging media/webrtc/signaling/src/sdp/RsdparsaSdpInc.h
merging media/webrtc/signaling/src/sdp/RsdparsaSdpMediaSection.cpp
merging media/webrtc/signaling/src/sdp/rsdparsa/src/media_type.rs
merging media/webrtc/signaling/src/sdp/rsdparsa_capi/src/media_section.rs
warning: conflicts while merging media/webrtc/signaling/gtest/sdp_unittests.cpp! (edit, then use 'hg resolve --mark')
warning: conflicts while merging media/webrtc/signaling/src/sdp/rsdparsa/src/media_type.rs! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 7

[Johannes Willbold]

Comment on attachment 8983932 [details]
Bug 1438289: Implemented RsdparsaSdpMediaSection::AddCodec and ::ClearCodecs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/249786/diff/3-4/

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9ef848655b33
Implemented RsdparsaSdpMediaSection::AddCodec and ::ClearCodecs. r=dminor

Comment 9

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/9ef848655b33