Skip to content

Conversation

@tkanakamalla
Copy link

@tkanakamalla tkanakamalla commented Jun 24, 2025

Fixes #1

  • enums
  • rtpmap
  • fmtp
  • rtcp
  • rtcp-fb
  • extmap
  • fingerprint
  • group
  • candidate
  • crypto
  • ssrc
  • setup
  • direction
  • squash/fixup the commits as required

@tkanakamalla tkanakamalla marked this pull request as draft June 24, 2025 15:19
Copy link
Owner

@sdroege sdroege left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise looks good to me

@tkanakamalla tkanakamalla force-pushed the strings_to_enums branch 2 times, most recently from d9257ff to 5286f1a Compare July 7, 2025 12:17
@sdroege
Copy link
Owner

sdroege commented Jul 8, 2025

clippy should be clean if you rebase on top of latest main

@tkanakamalla tkanakamalla force-pushed the strings_to_enums branch 2 times, most recently from 703493f to a9c14b7 Compare July 8, 2025 12:06
@tkanakamalla tkanakamalla force-pushed the strings_to_enums branch 2 times, most recently from 0790c2d to 017b97f Compare July 31, 2025 19:25
and write new methods to access the parsed SDP members as enums
/// Payload type, a numerical value between 0 and 127
pub pt: u8,
/// Name of the encoding
// TODO: is it useful to have an enum for all known encoding?
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potentially useful, just like for the registered payload types. But in this struct here, a string / u8 is the correct thing. You could have functions on the struct that try to handle pt / encoding with those enums though

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://gitlab.freedesktop.org/gstreamer/gst-plugins-rs/-/blob/44a632811e6718909409b5c229508072a1447e62/net/rtsp/src/rtspsrc/sdp.rs#L215 basically having guess_rtpmap_from_pt() and guess_rtpmap_from_encoding_name() here in some form seems useful.

@nirbheek Do you remember why your code allows rtpmap without clock-rate? The SDP RFC does not allow that

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are payload types for which clock-rate is static, and tools like ffmpeg won't add the clock-rate in those cases. For example pt=10/11 (L16 2ch/1ch), see table in https://en.wikipedia.org/wiki/RTP_payload_formats#Audio_and_video_payload_types.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. ffmpeg is wrong to not add it of course, specifying the clock rate in the SDP is not optional.

@sdroege sdroege mentioned this pull request Aug 5, 2025
@tkanakamalla
Copy link
Author

   Compiling sdp-types v0.1.8 (/home/runner/work/sdp-types/sdp-types)
error[E0106]: missing lifetime specifier
   --> src/lib.rs:363:17
    |
363 |     const NAME: &str;
    |                 ^ expected named lifetime parameter
    |
help: consider introducing a named lifetime parameter
    |
362 ~ pub trait TypedAttribute<'a>: Display + FromStr<Err = AttributeErr> {
363 ~     const NAME: &'a str;
    |

I wonder why the other tests or my local build did not report this error

@sdroege
Copy link
Owner

sdroege commented Jan 7, 2026

I wonder why the other tests or my local build did not report this error

Because you don't build with Rust 1.65

@tkanakamalla
Copy link
Author

So is it a shortcoming of Rust 1.65? Why do we have a test with that specific version?

@sdroege
Copy link
Owner

sdroege commented Jan 7, 2026

Because that's the minimum version supported here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Change various string fields to more expressive types

4 participants