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

fix(KVS): - Add Support for More Resolution Widths #3609

Merged
merged 12 commits into from
Jul 29, 2024

Conversation

stefankiesz
Copy link
Contributor

Issue

When streaming to Kinesis Video Streams using this SDK, the resolution of the frame Image read from the Android camera may not match up with the resolution configured by the user's application, causing completely distorted frames to be ingested:

image

This is due to a standard memory layout optimization when processing images. Typically, an image's width must be a multiple of a value such as, for example, 64 in order to line up with the optimal line/row stride of the image. If the image width is not a multiple of 64, padding is added to get the width up to the nearest multiple of 64. However, the encoder would still be configured for the user-defined resolution's width, thus causing such image stride distortions.

From my testing with the AmazonKinesisVideoDemoApp on a Samsung Galaxy S22 Ultra, the selected resolution must indeed be a multiple of 64 in order for frames to come into KVS undistorted. Here are some results:

176 x 144 - broken
320 x 240 - good
352 x 288 - broken
640 x 360 - good
640 x 480 - good
720 x 480 - broken
960 x 720 - good
1088 x 1088 - good
1280 x 720 - good
1440 x 1080 - broken
1920 x 824 - good
1920 x 1080 - good
1440 x 1080 - broken

Now with some more details added, the table below shows the frame width mismatch between the output buffers of the Images coming in from the camera and the encoder's input buffers:

image

Solution

The solution here is to manually check for whether the Image rowStride is not equivalent to the Image width. If not equivalent, then ignore the padding when copying the ByteBuffers from the outputted Image to the encoder's input ByteBuffers. So, in the case where there is image padding to ignore, for each frame, we have to make h copy operations instead of just 1 where h is the height of the image (h is the row count).

Testing

These changes were tested by building this SDK locally and running with the AmazonKinesisVideoDemoApp. The video footage was ingested without distortion for all tested resolutions. There was a slight drop in frame rate for the DemoApp's video preview, likely due to the additional copy operations.


By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

* MIcrophoneFrameSource builds, runs with "failed sampleRate 8000" exception

* Fix all runtime errors

* Started on 2-track streamInfo

* Construct TrackInfo, build errors fixed

* Progress, facing STATUS_MKV_INVALID_FRAME_DATA now if include audio with video

* Working, missing CPD error on playback though

* Works!! Todo: Should up the sample rate

* Begin modularizing code, not yet built nor test run

* Cleanup spaces, add to createFrameWithTrackID log

* Cleanup diffs

* Fix some importing and naming

* Rename AndroidAudioVideoMediaSource

* Add configuration class, control audio start/stop, working

* Saving attempted solutions to the issue

* Add TimeStampProvider to enforce unique timestamps

* Not cleaning junk, saving working camera 90deg rotation!

* Saving progress of fixing semi-planar case

* Fix convertByteArrayToPlanes, conversions now working perfect for semiplanar, rotation still not

* Rotation and colors work!

* Optimized rotation working, cleaned up comments.

* Turn off rotation, cleanup
@stefankiesz stefankiesz requested a review from a team as a code owner July 11, 2024 00:19
@stefankiesz stefankiesz changed the title Add Support for More Resolution Widths KVS - Add Support for More Resolution Widths Jul 11, 2024
Copy link
Member

@mattcreaser mattcreaser left a comment

Choose a reason for hiding this comment

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

Thanks for the well-commented code, this is easy to follow. Just a couple of small syntax cleanups needed.

Co-authored-by: sirknightj <sirknightj@gmail.com>
Co-authored-by: Matt Creaser <mattcreaser@gmail.com>
@stefankiesz
Copy link
Contributor Author

@sirknightj @mattcreaser, thanks for taking a look folks! I just merged in all your suggested changes.

…econnectors/kinesisvideo/encoding/EncoderFrameSubmitter.java

Co-authored-by: sirknightj <sirknightj@gmail.com>
@stefankiesz
Copy link
Contributor Author

Looks like we need 1 more maintainer approval for merging whenever anyone gets the chance, thanks.

@mattcreaser mattcreaser enabled auto-merge (squash) July 29, 2024 12:54
@mattcreaser mattcreaser changed the title KVS - Add Support for More Resolution Widths feat(KVS): - Add Support for More Resolution Widths Jul 29, 2024
@mattcreaser mattcreaser changed the title feat(KVS): - Add Support for More Resolution Widths fix(KVS): - Add Support for More Resolution Widths Jul 29, 2024
@mattcreaser mattcreaser merged commit 9bae96f into aws-amplify:main Jul 29, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants