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 RTP VP9 Reader Test #114

Closed
wants to merge 1 commit into from

Conversation

ManishaJajoo
Copy link
Contributor

Change-Id: Idcc6dcb62172b71465c30d8396b8a20533cc47b6

Change-Id: Idcc6dcb62172b71465c30d8396b8a20533cc47b6
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.

Some refactoring comments. Please refer to the RtpAmrReaderTest for the style that we prefer, thanks!

private RtpVp9Reader vp9Reader;
private FakeTrackOutput trackOutput;
@Mock
private ExtractorOutput extractorOutput;
Copy link
Contributor

Choose a reason for hiding this comment

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

There's no need to mock ExtractorOutput, you can use FakeExtractorOutput.


private ParsableByteArray packetData;

private RtpVp9Reader vp9Reader;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think there's a need to keep the vp9Reader as a field, you can just instantiate one in each test. Having it as a test creates the illusion that there's shared state.

See also my comment about the consume method.

assertThat(trackOutput.getSampleTimeUs(1)).isEqualTo(3200);
}

private static RtpPacket createRtpPacket(
Copy link
Contributor

Choose a reason for hiding this comment

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

It's unnecessary to wrap a class that already has a Builder in a method with many parameters, no? Please consider removing this method.

}

private void consume(RtpPacket rtpPacket) {
packetData.reset(rtpPacket.payloadData);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you make this method into static, and it takes a vp9Reader as parameter? You can also just create a new ParsablyByteArray every time this method is called.

This way you can eliminate the need for vp9Reader and packetData as field.

/* timestamp= */ 2599168056L,
/* sequenceNumber= */ 40289,
/* marker= */ false,
/* payloadData= */ getBytesFromHexString("08000102030405060708090A"));
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you extend this into something like this for better readability?

byte[] frame1Fragment1Data = getBytesFromHexString("000102...");
RtpPacket frame1fragment1 = new RtpPacket.Builder().
                                                       // ...other setters
                                                       .setPayloadData(Bytes.concat(
                                                         // header
                                                         getBytesFromHexString("0800"),
                                                         frame1Fragment1Data))
                                                       .build();
byte[] frame1Data = Bytes.concat(frame1Fragment1Data, frame1Fragment2Data);

@RunWith(AndroidJUnit4.class)
public final class RtpVp9ReaderTest {

private final RtpPacket frame1fragment1 =
Copy link
Contributor

Choose a reason for hiding this comment

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

The f in fragment should be in upper case.

Copy link
Contributor

Choose a reason for hiding this comment

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

My bad, the names should be FRAME_1_FRAGMENT_1, and please make these fields static final too, thanks!

@claincly
Copy link
Contributor

NVM, I'll merge this one with #115 and change the styles.

@claincly claincly closed this Jul 12, 2022
@ManishaJajoo
Copy link
Contributor Author

@claincly , apologies for the delay, the changes were in internal review. We have added the changes in our branch. You can take a look here: ittiam-systems@a7adf75

@claincly
Copy link
Contributor

No worries, thanks for making the change.

@androidx androidx locked and limited conversation to collaborators Mar 3, 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.

2 participants