-
Notifications
You must be signed in to change notification settings - Fork 471
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for RTSP Opus #53
Conversation
Added Opus RTP packet reader and added support for Opus playback through RTSP Change-Id: Ib6702bd8aafd0bd782e89127ab907061ff06ccb3
libraries/exoplayer_rtsp/src/main/java/androidx/media3/exoplayer/rtsp/RtpPayloadFormat.java
Outdated
Show resolved
Hide resolved
libraries/exoplayer_rtsp/src/main/java/androidx/media3/exoplayer/rtsp/RtspMediaTrack.java
Outdated
Show resolved
Hide resolved
// RFC7587 Section 6.1 | ||
// the RTP timestamp is incremented with a 48000 Hz clock rate | ||
// for all modes of Opus and all sampling rates. | ||
checkArgument(clockRate == 48000, "Invalid sampling rate"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe define a public constant in the reader and reference it here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RtpOpusReader is not visible to RtspMediaTrack as the readers belong to a different package and are default
Added the constant to this class itself
libraries/exoplayer_rtsp/src/main/java/androidx/media3/exoplayer/rtsp/reader/RtpOpusReader.java
Show resolved
Hide resolved
libraries/exoplayer_rtsp/src/main/java/androidx/media3/exoplayer/rtsp/reader/RtpOpusReader.java
Outdated
Show resolved
Hide resolved
libraries/exoplayer_rtsp/src/main/java/androidx/media3/exoplayer/rtsp/reader/RtpOpusReader.java
Outdated
Show resolved
Hide resolved
libraries/exoplayer_rtsp/src/main/java/androidx/media3/exoplayer/rtsp/reader/RtpOpusReader.java
Outdated
Show resolved
Hide resolved
libraries/exoplayer_rtsp/src/main/java/androidx/media3/exoplayer/rtsp/reader/RtpOpusReader.java
Outdated
Show resolved
Hide resolved
libraries/exoplayer_rtsp/src/main/java/androidx/media3/exoplayer/rtsp/reader/RtpOpusReader.java
Show resolved
Hide resolved
libraries/exoplayer_rtsp/src/main/java/androidx/media3/exoplayer/rtsp/reader/RtpOpusReader.java
Outdated
Show resolved
Hide resolved
Merged internally. The issue will be marked as merged automatically when we do a push. Thanks for the contribution! |
Change-Id: I189811c9bef9d11e93472c755bc19dee5dc3ee7c
* |ID Header| | Comment Header | |Audio Data Packet 1 | | ... | ||
* +---------+ +----------------+ +--------------------+ +----- | ||
*/ | ||
if (!foundOpusIDHeader) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for not getting back to this sooner, I thought we have merged it.
All the logic here to find Opus ID and comment headers imply that the header and comment header is ONE PACKET long. But that is not what is specified in the RFC.
According to the RFC, the ID header is one PAGE long, and the comment header can span across many pages. And one page can span multiple RTP packets. Also, "Audio data packets might span page boundaries. The first audio data page could have the 'continued packet' flag set"
Although this is working right now, it's not semantically correct. So please try to make it compliant with the RFC by supporting multiple packet pages. There are examples of one access unit spans multiple RTP packets, like in the H264/265 reader. It's probably hard to test, but given you have some tests going on, please add some test based on your interpretation to the RFC.
* |ID Header| | Comment Header | |Audio Data Packet 1 | | ... | ||
* +---------+ +----------------+ +--------------------+ +----- | ||
*/ | ||
if (!foundOpusIDHeader) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for not getting back to this sooner, I thought we have merged it.
All the logic here to find Opus ID and comment headers imply that the header and comment header is ONE PACKET long. But that is not what is specified in the RFC.
According to the RFC, the ID header is one PAGE long, and the comment header can span across many pages. And one page can span multiple RTP packets. Also, "Audio data packets might span page boundaries. The first audio data page could have the 'continued packet' flag set"
Although this is working right now, it's not semantically correct. So please try to make it compliant with the RFC by supporting multiple packet pages. There are examples of one access unit spans multiple RTP packets, like in the H264/265 reader. It's probably hard to test, but given you have some tests going on, please add some test based on your interpretation to the RFC.
PiperOrigin-RevId: 453490088 (cherry picked from commit a2a4504)
PiperOrigin-RevId: 453490088
PiperOrigin-RevId: 453490088 (cherry picked from commit ade3452)
Added Opus RTP packet reader and added support for Opus
playback through RTSP
Change-Id: Ib6702bd8aafd0bd782e89127ab907061ff06ccb3