webrtc-rs / webrtc

A pure Rust implementation of WebRTC
https://webrtc.rs
Apache License 2.0
4.08k stars 363 forks source link

feature(media/ogg_writer): add packet segments to page #499

Closed yinshanyang closed 12 months ago

yinshanyang commented 1 year ago

Implement the changes in https://github.com/pion/webrtc/pull/2531 which fixes https://github.com/pion/webrtc/issues/2472.

Description

PR adds packet segmentation capability to OggWriter. This makes it possible to accept payloads bigger than 255 bytes which are actually more common that I thought (e.g. Firefox seems to be generating quite a few). Without this change we'd be defaulting to a single segment causing data loss when writing and corrupted files as consequence.

There's still a theoretical limit of 255 segments per page that we are not handling (packets spanning pages) but for encoded audio data I am hoping that shouldn't ever become an issue.

In addition to Firefox, OBS 30.0 Beta 3 frequently produces payloads larger than 255 bytes, even at the lowest bitrate setting of 64 kbps.

codecov[bot] commented 1 year ago

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (71157ba) 61.56% compared to head (821cb5b) 61.50%.

:exclamation: Current head 821cb5b differs from pull request most recent head 324cde0. Consider uploading reports for the commit 324cde0 to get more accurate results

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #499 +/- ## ========================================== - Coverage 61.56% 61.50% -0.06% ========================================== Files 529 529 Lines 48839 48854 +15 Branches 12361 12380 +19 ========================================== - Hits 30066 30047 -19 - Misses 9601 9616 +15 - Partials 9172 9191 +19 ``` | [Files](https://app.codecov.io/gh/webrtc-rs/webrtc/pull/499?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=webrtc-rs) | Coverage Δ | | |---|---|---| | [media/src/io/ogg\_writer/ogg\_writer\_test.rs](https://app.codecov.io/gh/webrtc-rs/webrtc/pull/499?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=webrtc-rs#diff-bWVkaWEvc3JjL2lvL29nZ193cml0ZXIvb2dnX3dyaXRlcl90ZXN0LnJz) | `75.00% <72.22%> (-1.93%)` | :arrow_down: | ... and [21 files with indirect coverage changes](https://app.codecov.io/gh/webrtc-rs/webrtc/pull/499/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=webrtc-rs)

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

rainliu commented 1 year ago

Please fix clippy/fmt

yinshanyang commented 1 year ago

Please fix clippy/fmt

Seems like the clippy/fmt errors made their way into master before this pull request:

  Downloaded pkg-config v0.3.27
  Downloaded openssl-sys v0.9.93
  Downloaded openssl-macros v0.1.1
  Downloaded foreign-types v0.3.2
  Downloaded vcpkg v0.2.15
  Downloaded foreign-types-shared v0.1.1
  Downloaded openssl v0.10.57
  Downloaded openssl-src v300.1.5+3.1.3
  Downloaded 8 crates (9.4 MB) in 1.60s (largest was `openssl-src` at 8.8 MB)
   Compiling webrtc-util v0.8.0 (/Users/shanyang/Projects/sandbox-rtmp/webrtc/util)
   Compiling openssl-src v300.1.5+3.1.3
   Compiling pkg-config v0.3.27
   Compiling vcpkg v0.2.15
   Compiling openssl v0.10.57
    Checking foreign-types-shared v0.1.1
   Compiling openssl-macros v0.1.1
    Checking sdp v0.6.0 (/Users/shanyang/Projects/sandbox-rtmp/webrtc/sdp)
    Checking foreign-types v0.3.2
    Checking signal v0.1.0 (/Users/shanyang/Projects/sandbox-rtmp/webrtc/examples/examples/signal)
    Checking examples v0.5.0 (/Users/shanyang/Projects/sandbox-rtmp/webrtc/examples)
    Checking webrtc-constraints v0.1.0 (/Users/shanyang/Projects/sandbox-rtmp/webrtc/constraints)
   Compiling openssl-sys v0.9.93
    Checking stun v0.5.0 (/Users/shanyang/Projects/sandbox-rtmp/webrtc/stun)
    Checking webrtc-dtls v0.8.0 (/Users/shanyang/Projects/sandbox-rtmp/webrtc/dtls)
    Checking rtp v0.9.0 (/Users/shanyang/Projects/sandbox-rtmp/webrtc/rtp)
    Checking webrtc-mdns v0.6.0 (/Users/shanyang/Projects/sandbox-rtmp/webrtc/mdns)
    Checking webrtc-sctp v0.9.0 (/Users/shanyang/Projects/sandbox-rtmp/webrtc/sctp)
    Checking rtcp v0.10.0 (/Users/shanyang/Projects/sandbox-rtmp/webrtc/rtcp)
    Checking turn v0.7.0 (/Users/shanyang/Projects/sandbox-rtmp/webrtc/turn)
    Checking webrtc-media v0.7.0 (/Users/shanyang/Projects/sandbox-rtmp/webrtc/media)
    Checking webrtc-data v0.8.0 (/Users/shanyang/Projects/sandbox-rtmp/webrtc/data)
    Checking webrtc-ice v0.10.0 (/Users/shanyang/Projects/sandbox-rtmp/webrtc/ice)
    Checking hub v0.1.0 (/Users/shanyang/Projects/sandbox-rtmp/webrtc/dtls/examples/hub)
error: use of deprecated method `chrono::DateTime::<Tz>::timestamp_nanos`: use `timestamp_nanos_opt()` instead
  --> rtp/src/extension/abs_send_time_extension/abs_send_time_extension_test.rs:39:49
   |
39 |             .checked_add(Duration::from_nanos(t.timestamp_nanos() as u64))
   |                                                 ^^^^^^^^^^^^^^^
   |
   = note: `-D deprecated` implied by `-D warnings`

error: use of deprecated method `chrono::DateTime::<Tz>::timestamp_nanos`: use `timestamp_nanos_opt()` instead
  --> rtp/src/extension/abs_send_time_extension/abs_send_time_extension_test.rs:58:49
   |
58 |             .checked_add(Duration::from_nanos(t.timestamp_nanos() as u64))
   |                                                 ^^^^^^^^^^^^^^^

error: use of deprecated method `chrono::DateTime::<Tz>::timestamp_nanos`: use `timestamp_nanos_opt()` instead
  --> rtp/src/packetizer/packetizer_test.rs:44:49
   |
44 |             .checked_add(Duration::from_nanos(t.timestamp_nanos() as u64))
   |                                                 ^^^^^^^^^^^^^^^

error: could not compile `rtp` (lib test) due to 3 previous errors
warning: build failed, waiting for other jobs to finish...

Should I attempt to fix the errors there? As a heads up, I have not looked into rtp at all before.