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

Rtp mpeg4 #35

Merged
merged 6 commits into from
Apr 11, 2022
Merged

Rtp mpeg4 #35

merged 6 commits into from
Apr 11, 2022

Conversation

ManishaJajoo
Copy link
Contributor

Added support for RTP Mpeg4Reader in Exoplayer

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

Change-Id: I57c9a61b18471dbd2c368177ebfb89ee662f995b
@tonihei
Copy link
Collaborator

tonihei commented Feb 1, 2022

@ManishaJajoo We can only accept PRs if you sign the CLA. Could you please do that so we can take a look? Thanks!

@claincly Assigning to you to further handle the PR once the CLA is signed.

Copy link
Contributor

@claincly claincly left a comment

Choose a reason for hiding this comment

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

Mostly nits

@claincly
Copy link
Contributor

claincly commented Feb 21, 2022

Please also add test to RtspMediaTrackTest, targeting processing of the codec specific parsing logic, like the ones added in

  • CodecSpecificDataUtil
  • RtspMediaTrack, especially for MPEG4_CODECS, because there was a comment about the missing period.

Also please consider adding the test for RtpMPEG4Reader, for the logics like getBufferFlagsFromVop

You can submit the test as a separate PR, thanks!

@ManishaJajoo
Copy link
Contributor Author

Mostly nits

resolved the changes requested

CodecSpecificDataUtil.getVideoResolutionFromMpeg4VideoConfig(configBuffer);
formatBuilder.setWidth(resolution.first).setHeight(resolution.second);
} else {
// set the default width and height
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is the default width and height 352x288? Is it in an RFC? If so please add a link

Copy link
Contributor Author

@ManishaJajoo ManishaJajoo Mar 4, 2022

Choose a reason for hiding this comment

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

The default resolution is not mentioned in RFC.
For non-standard fmtp where configinput is missing, we use 352X288 as default since CIF 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/mpeg4_h263/C2SoftMpeg4Dec.cpp;l=130

@claincly
Copy link
Contributor

claincly commented Mar 4, 2022

As mentioned in #35 (comment), please add a simple test.

@ManishaJajoo
Copy link
Contributor Author

Sure, we will submit the test as a separate PR.

@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 f48babb 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
5 participants