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 VP8 #47

Merged
merged 3 commits into from
Apr 11, 2022
Merged

Add support for RTSP VP8 #47

merged 3 commits into from
Apr 11, 2022

Conversation

rakeshnitb
Copy link
Contributor

Added VP8 RTP packet reader and added support for VP8 playback
through RTSP.

Change-Id: Ie22ab79a234f61681cf95886a6ed8104a742f056

Added VP8 RTP packet reader and added support for VP8 playback
through RTSP.

Change-Id: Ie22ab79a234f61681cf95886a6ed8104a742f056
Change-Id: I2c8eb8d6ee28d8c044d71db042f3b186ea5762f3
@@ -56,6 +56,10 @@

private static final String GENERIC_CONTROL_ATTR = "*";

/** Default width and height for VP8. */
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add comment on where these defaults are defined.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

RFC doesn't mandate any codec specific data(like width, height) to be present in fmtp attributes for VP8.
RFC also doesn't mentioned any default resolution. So, i use 320X240 as default since it is the default resolution used by Android Software decoder.
Adding the link for your reference:
https://cs.android.com/android/platform/superproject/+/master:frameworks/av/media/codec2/components/vpx/C2SoftVpxDec.cpp;drc=749a74cc3e081c16ea0e8c530953d0a247177867;l=70

Format trackFormat = payloadFormat.format;
Format.Builder formatBuilder = trackFormat.buildUpon();
formatBuilder.setWidth(width).setHeight(height);
trackOutput.format(formatBuilder.build());
Copy link
Contributor

Choose a reason for hiding this comment

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

Double checking: If playing back a 1080p VP8 video, the player is able to pick up the 1080p format information? Because no other RTP readers reconfigures track format.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have checked that the player is able to pick format information for 1080p.

firstReceivedTimestamp = C.TIME_UNSET;
previousSequenceNumber = C.INDEX_UNSET;
fragmentedSampleSizeBytes = 0;
gotFirstPacketOfVP8Frame = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

receivedFirstVp8FramePacket?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In VP8 a frame can be divided into partitions, these partitions can be sent as a packet. This variable is used to check that the packet received is the first packet of a frame.


/** Creates an instance. */
public RtpVP8Reader(RtpPayloadFormat payloadFormat) {
this.payloadFormat = payloadFormat;
Copy link
Contributor

Choose a reason for hiding this comment

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

please order these to the declaration order. Also initialize startTimeOffsetUs to C.TIME_UNSET

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you elaborate on the order you want to keep because as for my knowledge variables are in the same order of declaration.
I can't initialize "startTimeOffsetUs" to C.TIME_UNSET because value of "C.TIME_UNSET" is min of Long. For any clip startTimeOffsetUs need to zero except if we are seeking.

// Parsing frame data to get width and height, RFC6386 Section 9.1
int currPosition = data.getPosition();
data.setPosition(currPosition + 6);
int width = data.readLittleEndianUnsignedShort() & 0x3fff;
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need littleEndian here? I assume the data here is in "network order". Also this library could be used on big-endian devices, will using littleEndian method here limit the usecases?

Copy link
Contributor Author

@rakeshnitb rakeshnitb Mar 8, 2022

Choose a reason for hiding this comment

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

According to RFC6386 Section 19.1 width and height are save in littleEndianess.
https://datatracker.ietf.org/doc/html/rfc6386#section-19.1

Change-Id: Id47c746b199831d0bb51dc736c43fd20c2e79c08
@claincly
Copy link
Contributor

Merged internally. The issue will be marked as merged automatically when we do a push. Thanks for the contribution!

@icbaker icbaker merged commit 9f29d22 into androidx:main Apr 11, 2022
icbaker added a commit that referenced this pull request May 9, 2022
@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

3 participants