Skip to content
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

Merged
merged 5 commits into from
Jun 9, 2022
Merged

Conversation

ManishaJajoo
Copy link
Contributor

Added Opus RTP packet reader and added support for Opus
playback through RTSP

Change-Id: Ib6702bd8aafd0bd782e89127ab907061ff06ccb3

Added Opus RTP packet reader and added support for Opus
playback through RTSP

Change-Id: Ib6702bd8aafd0bd782e89127ab907061ff06ccb3
// 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");
Copy link
Contributor

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?

Copy link
Contributor Author

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

@claincly
Copy link
Contributor

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) {
Copy link
Contributor

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) {
Copy link
Contributor

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.

@marcbaechinger marcbaechinger merged commit a2a4504 into androidx:main Jun 9, 2022
marcbaechinger added a commit that referenced this pull request Jun 15, 2022
PiperOrigin-RevId: 453490088
(cherry picked from commit a2a4504)
marcbaechinger added a commit that referenced this pull request Sep 30, 2022
marcbaechinger added a commit that referenced this pull request Sep 30, 2022
PiperOrigin-RevId: 453490088
(cherry picked from commit ade3452)
@androidx androidx locked and limited conversation to collaborators Mar 4, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants