Skip to content

Make minimum set of API methods in JNI layer to support encoding/decoding#563

Merged
hrosa merged 8 commits intogithub-107from
github-550
Oct 5, 2017
Merged

Make minimum set of API methods in JNI layer to support encoding/decoding#563
hrosa merged 8 commits intogithub-107from
github-550

Conversation

@morosev
Copy link
Contributor

@morosev morosev commented Oct 1, 2017

No description provided.

@morosev morosev added the DSP label Oct 1, 2017
@morosev morosev added this to the 7.0M - sprint06 milestone Oct 1, 2017
@morosev morosev requested a review from hrosa October 1, 2017 23:07
#include <stdlib.h>
#include <string.h>

#define FRAME_SIZE 480
Copy link
Contributor

Choose a reason for hiding this comment

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

Open ticket to make these variables configurable in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The hardcoded values will be substituted by configurable values in task #551


try {
FileInputStream inputStream = new FileInputStream("src\\main\\jni\\test_sound_mono_48.pcm");
FileOutputStream outputStream = new FileOutputStream("src\\main\\jni\\test_sound_mono_48_decoded.pcm", false);
Copy link
Contributor

Choose a reason for hiding this comment

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

Create temporary file instead and make sure we delete it after the test.

Example: Path tempFile = Files.createTempFile("tempfiles", ".tmp");

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

opus.closeNative();
}
} catch (IOException exc) {
System.out.println("IOException: " + exc.getMessage());
Copy link
Contributor

Choose a reason for hiding this comment

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

Use log4j instead with ERROR threshold

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

}
} catch (IOException exc) {
System.out.println("IOException: " + exc.getMessage());
return;
Copy link
Contributor

Choose a reason for hiding this comment

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

Fail the test instead of silently ignoring the error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@@ -71,9 +79,36 @@ public void tearDown() throws Exception {
*/
@Test
public void testCodec() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Test is missing assertions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

opus.sayHelloNative();

try {
FileInputStream inputStream = new FileInputStream("src\\main\\jni\\test_sound_mono_48.pcm");
Copy link
Contributor

Choose a reason for hiding this comment

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

Move resource to src/test/resources/...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@morosev
Copy link
Contributor Author

morosev commented Oct 2, 2017

@hrosa It is ready to be merged.

FileOutputStream outputStream = new FileOutputStream(outputFile, false);

OpusJni opus = new OpusJni();
opus.setOpusObserverNative(this);
Copy link
Contributor

@hrosa hrosa Oct 3, 2017

Choose a reason for hiding this comment

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

Create a Mockito.mock for OpusJni.Observer instead of using this reference

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@@ -42,6 +50,9 @@
*
*/
public class OpusCodecTest implements OpusJni.Observer {
Copy link
Contributor

Choose a reason for hiding this comment

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

Test class should not implement OpusJni.Observer

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

}

@Override
public void onHello() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Delete this method

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Deleted

}

assertTrue(testPassed);

Copy link
Contributor

Choose a reason for hiding this comment

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

Mockito.verify(opusObserver).onHello()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done


boolean testPassed = false;

try {
Copy link
Contributor

Choose a reason for hiding this comment

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

For safety, use try-with-resources for any closeable resource used (inputStream, outputStream).
https://docs.oracle.com/javase/tutorial/essential/exceptions/tryResourceClose.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@hrosa hrosa merged commit 6e2e3e1 into github-107 Oct 5, 2017
@hrosa hrosa removed the Peer Review label Oct 5, 2017
@hrosa hrosa deleted the github-550 branch October 5, 2017 08:32
@hrosa hrosa added the unplanned label Oct 5, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants