-
Notifications
You must be signed in to change notification settings - Fork 336
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
Rework the ReadMe to be more quick-start friendly #1173
base: develop-pre-3.4.1
Are you sure you want to change the base?
Conversation
* `pkg-config` for _Mac_/_Linux_, `pkgconfiglite` for _Windows_ | ||
* `m4` | ||
* _Windows_ only: `nasm` and `strawberryperl` | ||
|
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.
Suggest adding the installation commands for these.
sudo apt-get update
sudo apt-get install git cmake ...
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.
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.
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.
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.
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.
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.
|
||
If you are building on Windows you need to generate `NMake Makefiles`, you should run `cmake .. -G "NMake Makefiles"` | ||
_Mac and Linux_ |
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.
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
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.
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.
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.
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 ReportAttention: Patch coverage is
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. |
…n the Quick Start section
0dfaf7f
to
6577535
Compare
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 |
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.
For faster download, let's clone only the master branch.
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 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.
<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`. |
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 think this is confusing, you can explain this a bit better
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.
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.
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.
Yeah just remove it. It's already explained in docs/structures.md
_Windows_ | ||
```bat | ||
choco install gstreamer --version=1.22.8 | ||
choco install gstreamer-devel --version=1.22.8 |
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.
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?
|
||
### 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 |
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.
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?
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.
If you could add a note for that one, that would be great.
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
Future Improvements
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.