Rework the ReadMe to be more quick-start friendly #1173

stefankiesz · 2024-05-06T05:28:24Z

What was changed?
The ReadMe was re-worked to be more quick-start friendly. Missing information was added and made into tables. Typos and grammar issues were addressed.
(More details in the Overview section below.)

Why was it changed?
To improve the ReadMe and correct outstanding issues.

How was it changed?
By modifying the ReadMe document to address the above issues.

What testing was done for the changes?
The document was pasted into a spelling and grammar checker.

Overview

Add a collapsible vertical table of contents
Add a Quick Start section
Extend samples instructions
Add missing Windows instructions
Fix incorrect dependency installation commands
Add more links to resources and references
- New links include: AWS KVS getting started page, AWS KVS FAQs page, GStreamer element docs, Java Producer SDK
- Also added a link to Producer C in the Key Features section incase user does not need JNI nor GStreamer features
Make the CMake Arguments section into a table and fix typos in the descriptions
Make the kvssink parameters into a table, fix missing links, add in aws-region parameter
Mention and show how to stream from file with the kvssink sample
To the Quick Start section, add instructions for how/where to view the footage from the samples and link troubleshoot guides
...more misc. changes

Future Improvements

Consider splitting the QuickStart instructions for building the SDK into build-with-dependencies a build-using-system-dependencies and demonstrate how to install the system dependencies and the correct versions.
Improve upon and add to the Debugging/Troubleshooting section - likely should add more links to AWS docs. Specify Java version for JNI. Also should probably add a link to Java SDK and instructions if any are missing on that repo's ReadMe.
Add installation commands to all the prerequisite packages such as git and cmake. This will likely only fit well if the QuickStart section gets restructured into 3 separate sections for each OS.
Add a "Running Tests" section to show how to build and run the tests. Include details such as AWS authentication and how resources are managed when tests are running and failing.

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

sirknightj · 2024-05-07T16:05:29Z

README.md

+* `pkg-config` for _Mac_/_Linux_, `pkgconfiglite` for _Windows_
+* `m4`
+* _Windows_ only: `nasm` and `strawberryperl`
+


Suggest adding the installation commands for these.

sudo apt-get update sudo apt-get install git cmake ...

This seems like it would be unnecessarily verbose since these are very common packages which are likely already present on users' machines, but I'll add it and see how we feel about it.

On second thought, it definitely would be too verbose to have all these commands for all 3 OSs, but it should fit well if we restructure the QuickStart section in a future PR to be 3 separate sections for each OS. Will leave as is for now.

This seems like it would be unnecessarily verbose since these are very common packages which are likely already present on users' machines, but I'll add it and see how we feel about it.

In the case that it's not or if I want to verify for myself that it is installed? This information seems quite important to have.

sirknightj · 2024-05-07T16:07:53Z

README.md


-If you are building on Windows you need to generate `NMake Makefiles`, you should run `cmake .. -G "NMake Makefiles"`
+_Mac and Linux_


Would suggest splitting this into 2 subsections:

Build from scratch (build with dependencies=ON)

Build using system libraries (build with dependencies=OFF). For this, you would also need to install the necessary dependencies using system package manager

Going to add this to the PR description as a future addition to consider, but I'm not set on doing this quite yet since for the smoothest onboarding experience we would recommend to do build dependencies to ensure a supported version of a dependency library is being used.

To add to that, it's not as simple as just including the install instructions with the correct versions: users might already have some of the dependency libraries installed with unsupported versions and there would be potential for CMake or pkgconfig to link the incorrect ones.

codecov-commenter · 2024-06-29T20:07:27Z

Codecov Report

Attention: Patch coverage is 0% with 105 lines in your changes missing coverage. Please review.

Project coverage is 16.30%. Comparing base (eeb392f) to head (03d8f84).
Report is 10 commits behind head on develop.

❗ Current head 03d8f84 differs from pull request most recent head 3bb045b

Please upload reports for the commit 3bb045b to get more accurate results.

Files	Patch %	Lines
samples/kvssink_gstreamer_sample.cpp	0.00%	50 Missing ⚠️
...mazonaws/kinesis/video/producer/jni/Parameters.cpp	0.00%	46 Missing ⚠️
src/gstreamer/gstkvssink.cpp	0.00%	8 Missing ⚠️
...s/video/producer/jni/KinesisVideoClientWrapper.cpp	0.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #1173      +/-   ##
===========================================
- Coverage    16.48%   16.30%   -0.18%     
===========================================
  Files           50       50              
  Lines         7019     7095      +76     
===========================================
  Hits          1157     1157              
- Misses        5862     5938      +76

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

README.md

…anup cmake table

…n the Quick Start section

sirknightj · 2024-07-04T22:46:17Z

README.md

-GStreamer and JNI is NOT built by default, if you wish to build both you MUST execute `cmake .. -DBUILD_GSTREAMER_PLUGIN=ON -DBUILD_JNI=TRUE`
+### Download
+```bash
+git clone https://github.com/awslabs/amazon-kinesis-video-streams-producer-sdk-cpp.git


For faster download, let's clone only the master branch.

I tested the difference cloning this on my home network. Cloning the full repo, it takes between 1 and 2 seconds to clone and takes up 14 Mb. With --single-branch in the command, it still takes between 1 and 2 seconds to clone and takes up 13 Mb.

I don't think these gains are worth the loss of readily being able to checkout remote branches.

README.md

sirknightj · 2024-07-04T22:50:00Z

README.md

-export LD_LIBRARY_PATH=`pwd`/open-source/local/lib
+./kvssink_intermittent_sample <stream-name> <testsrc or devicesrc (optional)>
+```
+Setting the source to `testsrc` will use [videotestsrc](https://gstreamer.freedesktop.org/documentation/videotestsrc/?gi-language=c) and to `devicesrc` will use [autovideosrc](https://gstreamer.freedesktop.org/documentation/autodetect/autovideosrc.html?gi-language=c). By default, kvssink uses "DEFAULT_STREAM" as the stream name, and the sample uses videotestsrc as the source. If a KVS stream with the provided or default name does not exist, the stream will automatically be created.


Should add the stream parameters (2 hours retention)

What's the relevance of that information here? I don't want to crowd all the details here as it is a "quick start" guide and it would be prone to being out of date if we don't update it here if the default parameters ever change. I'd prefer these details live in the AWS docs and not here in the GitHub ReadMe.

sirknightj · 2024-07-04T22:50:24Z

README.md

+<br>
+
+#### Running Samples in offline mode
+By default, the samples run in near-real-time mode. To set offline mode, set streamInfo.streamCaps.streamingType to `STREAMING_TYPE_OFFLINE`, where, `streamInfo` is of type `StreamInfo`, `streamCaps` is of type `StreamCaps` and `streamingType` is of type `STREAMING_TYPE`.


I think this is confusing, you can explain this a bit better

This I just copied over from the existing ReadMe, but looking at it now, this is not correct instructions as we don't expose a StreamInfo object in the samples. Instead, the streaming_type variable should be modified to change the streaming type. However, I don't see why this would ever need to be done if the sample already changes it to offline mode when streaming from file. I'll remove this section entirely if you agree.

sirknightj · 2024-07-04T22:52:30Z

README.md

+| BUILD_GSTREAMER_PLUGIN       | OFF           | Build the kvssink GStreamer plugin
+| BUILD_JNI                    | OFF           | Build C++ wrapper for JNI to expose the functionality to Java/Android
+| BUILD_DEPENDENCIES           | ON            | Build depending libraries from source
+| BUILD_TEST                   | OFF           | Build unit/integration tests, may be useful to confirm support for your device, to run tests:       `./tst/producerTest`


Should mention if credentials & region are required to run the tests & whether the resources get cleaned up or not in case of failing tests.

I feel this would be too verbose for this table. I will add including a "Running Tests" section to the PR description as a potential future addition where these details would fit in well.

I think that's a good idea. Having a section dedicated to how to run the tests and the sanitizers and view the results.

sirknightj · 2024-07-04T22:53:05Z

README.md

+| BUILD_DEPENDENCIES           | ON            | Build depending libraries from source
+| BUILD_TEST                   | OFF           | Build unit/integration tests, may be useful to confirm support for your device, to run tests:       `./tst/producerTest`
+| CODE_COVERAGE                | OFF           | Enable coverage reporting
+| COMPILER_WARNINGS            | OFF           | Enable all compiler warnings


Why is this off by default?

I wouldn't know as this was likely a decision made long ago, but I'd imagine it was to reduce build log verbosity.

Also, the COMPILER_WARNINGS flag does nothing in PIC, Producer C, and Producer CPP, yet we have it as an option in all 3 SDKs. It's not a built-in option as far as what I can see from a quick search.

Let's just remove this from the table? ...and from the CMakeLists in a separate PR.

README.md

sirknightj · 2024-07-04T22:56:41Z

README.md

+### Debugging
+
+#### Build Issues
+* If you are running into issues building libcurl on M1 Mac, you can try `brew unlink openssl`.
 * When building the JNI, if you run into a cmake error `Could NOT find JNI (missing: JAVA_INCLUDE_PATH JAVA_INCLUDE_PATH2 JAVA_AWT_INCLUDE_PATH)`, make sure Java is installed and your environment variables are set correctly:  


I believe also need java 8 or 11..

Improving upon the Debugging section is already in the PR description as an action item, this will fall in that scope as it will need testing and verification.

sirknightj · 2024-07-09T08:03:48Z

README.md

+_Windows_
+```bat
+choco install gstreamer --version=1.22.8
+choco install gstreamer-devel --version=1.22.8


How come the gstreamer version is only specified in the windows? Does mac and linux requires certain gstreamer versions? Is this the only supported gstreamer version for windows?

sirknightj · 2024-07-09T08:05:11Z

README.md


+### Build
+#### Prepare a build directory in the newly checked out repository:
 ```bash
 mkdir -p amazon-kinesis-video-streams-producer-sdk-cpp/build
 cd amazon-kinesis-video-streams-producer-sdk-cpp/build


Don't we need to clone it in the C:/ for windows due to file path length restrictions?

Also can we adjust this to clone only the master branch?

sirknightj reviewed May 7, 2024

View reviewed changes

sirknightj added the documentation label Jun 8, 2024

sirknightj reviewed Jul 3, 2024

View reviewed changes

README.md Outdated Show resolved Hide resolved

stefankiesz added 25 commits July 3, 2024 12:10

Start remodeling the ReadMe

c759770

Fix typo, add italics to OS names

b0fe466

add todo

bdddfbc

Finish up the body

22654d1

Update ToC

f43d017

Move stuff, fix typos

4e24a0c

Improve title

210310a

Add a line

832558d

Add link for cmake options

9e27b78

Fix ToC

b6be3df

Pi->Linux

aa6ce2e

Add a comma

a2aba1e

Address feedback

3052424

Add more Windows instructions

e92808e

Restructure Debugging section, add more links

a4380a5

Add some periods

f807757

Add link to Java SDK

9fdf80a

Add link to Producer C SDK in Key Features section

f9912ae

Convert cmake options list to table

824237b

Add kvssink parameters table, modify credential-path description, cle…

175e463

…anup cmake table

Add stream from file kvssink sample instructions

b7e884a

Add instructions for how/where to view the footage from the samples i…

9cb1972

…n the Quick Start section

Address comments

62c48dc

Spelling and grammar

a4864ee

Remove accidental link

6577535

stefankiesz force-pushed the develop-readme-rework branch from 0dfaf7f to 6577535 Compare July 3, 2024 19:11

Downgrade Mac CI GCC version 13->12

9347869

stefankiesz marked this pull request as ready for review July 3, 2024 19:21

sirknightj reviewed Jul 4, 2024

View reviewed changes

README.md Show resolved Hide resolved

sirknightj reviewed Jul 4, 2024

View reviewed changes

README.md Show resolved Hide resolved

sirknightj reviewed Jul 4, 2024

View reviewed changes

README.md Outdated Show resolved Hide resolved

sirknightj reviewed Jul 4, 2024

View reviewed changes

stefankiesz added 2 commits July 8, 2024 21:38

Add AWS Authentication to quickstart section

adcf3ec

megabytes -> mebibytes

c924131

stefankiesz requested a review from sirknightj July 9, 2024 05:14

sirknightj reviewed Jul 9, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework the ReadMe to be more quick-start friendly #1173

Rework the ReadMe to be more quick-start friendly #1173

stefankiesz commented May 6, 2024 •

edited

Loading

sirknightj May 7, 2024

stefankiesz Jul 3, 2024

stefankiesz Jul 3, 2024 •

edited

Loading

sirknightj Jul 9, 2024

sirknightj May 7, 2024

stefankiesz Jul 3, 2024

stefankiesz Jul 3, 2024 •

edited

Loading

codecov-commenter commented Jun 29, 2024

sirknightj Jul 4, 2024

stefankiesz Jul 9, 2024

sirknightj Jul 4, 2024

stefankiesz Jul 9, 2024

sirknightj Jul 4, 2024

stefankiesz Jul 9, 2024 •

edited

Loading

sirknightj Jul 4, 2024

stefankiesz Jul 9, 2024

sirknightj Jul 9, 2024

sirknightj Jul 4, 2024

stefankiesz Jul 9, 2024 •

edited

Loading

sirknightj Jul 4, 2024

stefankiesz Jul 9, 2024

sirknightj Jul 9, 2024

sirknightj Jul 9, 2024


		If you are building on Windows you need to generate `NMake Makefiles`, you should run `cmake .. -G "NMake Makefiles"`
		_Mac and Linux_

Rework the ReadMe to be more quick-start friendly #1173

Are you sure you want to change the base?

Rework the ReadMe to be more quick-start friendly #1173

Conversation

stefankiesz commented May 6, 2024 • edited Loading

Overview

Future Improvements

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stefankiesz Jul 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stefankiesz Jul 3, 2024 • edited Loading

Choose a reason for hiding this comment

codecov-commenter commented Jun 29, 2024

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stefankiesz Jul 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stefankiesz Jul 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stefankiesz commented May 6, 2024 •

edited

Loading

stefankiesz Jul 3, 2024 •

edited

Loading

stefankiesz Jul 3, 2024 •

edited

Loading

stefankiesz Jul 9, 2024 •

edited

Loading

stefankiesz Jul 9, 2024 •

edited

Loading