-
Notifications
You must be signed in to change notification settings - Fork 75
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 PutEventMetadata support to Java and JNI #193
base: develop-pre-1.12.1
Are you sure you want to change the base?
Conversation
src/main/java/com/amazonaws/kinesisvideo/internal/producer/StreamEventMetadata.java
Show resolved
Hide resolved
src/main/java/com/amazonaws/kinesisvideo/internal/producer/StreamEventMetadata.java
Outdated
Show resolved
Hide resolved
byte mNumberOfPairs; | ||
|
||
// Optional custom data name/value pairs. String lengths must be <= MKV_MAX_TAG_NAME_LEN as defined in the native code. | ||
@Nullable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure this is nullable? There's a null check in the API implementation in Producer C which would throw STATUS_NULL_ARG
if this was null
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not seeing any null check in Producer C SDK, and the API implementation I believe is in PIC which also does not have such a check, but that is because the pointers to the names and values arrays are initialized with fixed lengths on the struct, so there's no risk of them being null.
My thinking is for code-parity, we might want to allow passing no arrays into the PIC layer since this is technically supported if constructing the metadata struct using designated initialization as we do in the PIC unit tests:
StreamEventMetadata Meta{STREAM_EVENT_METADATA_CURRENT_VERSION, NULL, 1, {}, {}}
. And from my testing, calling putKinesisEventMetadata with the following has no errors:
StreamEventMetadata Meta{STREAM_EVENT_METADATA_CURRENT_VERSION, NULL, 1, NULL, NULL}
(the last 2 members are the names and values arrays).
src/main/java/com/amazonaws/kinesisvideo/internal/client/mediasource/MediaSourceSink.java
Show resolved
Hide resolved
src/main/java/com/amazonaws/kinesisvideo/java/mediasource/file/ImageFrameSource.java
Outdated
Show resolved
Hide resolved
src/main/java/com/amazonaws/kinesisvideo/internal/producer/StreamEventMetadata.java
Outdated
Show resolved
Hide resolved
|
||
public StreamEventMetadata(String imagePrefix, byte numberOfPairs, String names[], String values[]) { | ||
mVersion = STREAM_EVENT_METADATA_CURRENT_VERSION; | ||
mImagePrefix = imagePrefix; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a check (throw an exception) to verify numberOfPairs == names.length == values.length
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, why do we need to pass in numberOfpairs? You can just set that to names.length
right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doing either of these will be taking away from code parity as we do not have such checks in PIC, and if we did, we might want them to be solely handled there. I believe we currently leave most existing invalid arg checks up to PIC (would be redundant checking in both Java and C land).
src/main/java/com/amazonaws/kinesisvideo/internal/producer/StreamEventMetadata.java
Outdated
Show resolved
Hide resolved
src/main/java/com/amazonaws/kinesisvideo/internal/producer/StreamEventMetadata.java
Show resolved
Hide resolved
src/main/java/com/amazonaws/kinesisvideo/internal/producer/StreamEventMetadata.java
Outdated
Show resolved
Hide resolved
...ain/java/com/amazonaws/kinesisvideo/internal/producer/jni/NativeKinesisVideoProducerJni.java
Outdated
Show resolved
Hide resolved
...ain/java/com/amazonaws/kinesisvideo/internal/producer/jni/NativeKinesisVideoProducerJni.java
Show resolved
Hide resolved
* to all events included in THIS function call. | ||
* @throws ProducerException | ||
*/ | ||
private native void putKinesisVideoEventMetadata(long clientHandle, long streamHandle, final int event, final StreamEventMetadata streamEventMetadata) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
final
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which item are you referring to that should be final? The clientHandle and streamHandle are not handled as final in any of the other native methods by the way.
Brings
PutEventMetadata
support for the Java Producer SDK through the following changes:StreamEventMetadata
class andStreamEventType
enum.PutEventMetadata
inNativeKinesisVideoProducerStream
.ImageFrameSource
for the DemoApp to use.Testing
The following testing and validation was conducted for various STREAM_EVENT_TYPE (StreamEventType in Java end) values using a KVS stream configured for image generation and notifications.
A. IMAGE_GENERATION - Valid Inputs
1. 0 StreamEventMetadata Name/Value Pairs
The first S3 object received contains an image of the footage:
✅ This image matches the first frame contained in the set of test .h264 files:
Due to the specific format or packaging of our
.h264
test files, it was not readily possible to use an ffmpeg command to display the first frame as an image. An alternative technique was used to view the above first test frame which involved hardcoding the Java sample's image file index to remain at 0, so the first frame was constantly uploaded to KVS. Playing back the KVS stream at any recent point would then show that first frame.Note: Images will no longer be shown for the remaining test cases below due to redundancy.
✅ The S3 object contains no additional metadata as expected:
2. 1 StreamEventMetadata Name/Value Pair
✅ S3 saved image is valid.
✅ S3 metadata received as expected:
✅ The above successful tests were the case for pair counts from 1-9, but 10 failed as seen below.
3. 10 StreamEventMetadata Name/Value Pairs
❌ No image object in S3.
❌ No metadata.
PutEventMetadata fails with
0x52000074 STATUS_MAX_FRAGMENT_METADATA_COUNT
.NOTE: This uncovered that the true limit to name/value pairs in PIC is 9, or 8 if using an S3
imagePrefix
.4. Long-Running Test: 1 StreamEventMetadata Name/Value Pair
The following test was run for 11 hours:
✅ Images and tags were received in the S3 bucket after 11 hours, with no errors.
5. Null StreamEventMetadata
✅ S3 saved image is valid.
✅ The S3 object contains no additional metadata as expected.
B. IMAGE_GENERATION - Invalid Inputs
1. Invalid
Name
: "AWS"✅ Received the expected error status code:
0x52000077 | STATUS_INVALID_METADATA_NAME
2.
Name
Too LongLimited by MKV_MAX_TAG_NAME_LEN which is 128.
Testing using 128 characters:
✅ No error. The name/value pair is present in the S3 object:
Testing using 129 characters:
✅ Received the expected error status code:
0x5200008e | STATUS_INVALID_IMAGE_METADATA_KEY_LENGTH
3.
Value
Too LongLimited by MKV_MAX_TAG_VALUE_LEN which is 256.
Testing using 256 characters:
✅ No error. The name/value pair is present in the S3 object:
Testing using 257 characters:
✅ Received the expected error status code:
0x5200008f | STATUS_INVALID_IMAGE_METADATA_VALUE_LENGTH
4.
Names
is Empty, butnumberOfPairs
is> 0
✅ Received the expected error status code:
0x00000001 | STATUS_NULL_ARG
5.
Names
is Not Empty, butnumberOfPairs
is< Names.length
Using previously used arrays of 10 name/value pairs, but with numberOfPairs set to 1:
numberOfPairs
is NOT optional:C. NONE
✅ Received the expected error status code:
0x00000002 | STATUS_INVALID_ARG
D. NOTIFICATION
Notification functionality was tested using an SNS topic with an email subscription.
✅ Received the expected notification via email with the expected SNS payload:
E. LAST
✅ Received the expected error status code:
0x00000002 | STATUS_INVALID_ARG
F. Multiple StreamEventType
Using bitwise OR, IMAGE_GENERATION (1 = 001) was combined with NOTIFICATION (2 = 010) to equal 3 (011):
✅ Both, the S3 object containing an image with tags, and the SNS email notification were received:
G. Negative StreamEventType
Care must be take here as the Java int passed to the JNI can be negative, while the StreamEventType parameter is an unsigned integer in PIC. Checks were placed in both the JNI and Java layers to error out if StreamEventType is negative.
✅ Received the expected Java error due to Precondition check:
java.lang.IllegalArgumentException
After removing the Java Precondition check to validate the JNI check:
✅ Received the expected native exception:
2024-05-10 17:29:48,969 [pool-3-thread-1] ERROR c.a.k.j.c.KinesisVideoJavaClientFactory - [PIC] KinesisVideoClientWrapper - putKinesisVideoEventMetadata(): STREAM_EVENT_TYPE cannot be negative.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.