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

Add features matrix to the README #1593

Merged
merged 1 commit into from
Dec 14, 2020
Merged

Add features matrix to the README #1593

merged 1 commit into from
Dec 14, 2020

Conversation

Sean-Der
Copy link
Member

List most popular features/benefits of Pion WebRTC.

Resolves #1279

Co-authored-by: ZHENK [email protected]

@Sean-Der
Copy link
Member Author

Sean-Der commented Dec 12, 2020

Would really love peoples opinion on this. Did I miss anything important? Any points I make do you think don't matter?

@codecov
Copy link

codecov bot commented Dec 12, 2020

Codecov Report

Merging #1593 (8c1c6a6) into master (67826b1) will decrease coverage by 0.26%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1593      +/-   ##
==========================================
- Coverage   75.25%   74.99%   -0.27%     
==========================================
  Files          78       78              
  Lines        5610     5610              
==========================================
- Hits         4222     4207      -15     
- Misses       1020     1034      +14     
- Partials      368      369       +1     
Flag Coverage Δ
go 76.59% <ø> (-0.30%) ⬇️
wasm 70.42% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
sctptransport.go 77.09% <0.00%> (-3.36%) ⬇️
operations.go 91.66% <0.00%> (-2.78%) ⬇️
peerconnection.go 74.35% <0.00%> (-0.70%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 67826b1...8c1c6a6. Read the comment docs.

@Sean-Der
Copy link
Member Author

@albrow @backkem @adwpc @AeroNotix @Antonito @ashellunts @at-wat @audiolion @baiyufei @billylindeman @bshi @cgojin @cloudwebrtc @cryptix @decanus @enobufs @epes @hugoArregui @jeremija @jech @leewardbound @lherman-cs @masterada @maxhawkins @mission-liao @OrlandoCo @r-novel @rviscarra @RyanGordon @saljam @scorpionknifes @seppo0010 @sgotti @steebchen @TannerGabriel @tarrencev @tekig @thinkski @trivigy @wdouglass

Would this have improved/detracted from your first opinion of using Pion? I have gotten a lot of feedback that people have no idea what pion/webrtc contains so wanted to bring this to the top.

@Sean-Der
Copy link
Member Author

https://github.com/pion/webrtc/blob/eef6045bb0203f0223f809cf0187571297b6adf6/README.md is a link directly to the file to see it formatted.

README.md Outdated
* Easy integration with x264, libvpx, GStreamer and ffmpeg.
* [Simulcast](https://github.com/pion/webrtc/tree/master/examples/simulcast)
* [SVC](https://github.com/pion/rtp/blob/master/codecs/vp9_packet.go#L138)
* [NACK](https://github.com/pion/interceptor/issues/7)
Copy link
Member

Choose a reason for hiding this comment

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

why there is a link to an issue? it means not implemented?

Copy link
Member Author

@Sean-Der Sean-Der Dec 12, 2020

Choose a reason for hiding this comment

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

Almost there! It is in a PR here but not merged yet.

Bandwidth Estimation is in Ion and I am hoping we can just move into interceptors soon!

Forward Error Correction isn't available at all yet :(

Do you think it is better to leave these out? I will update the NACK link, but was going to link to those issues to track them until complete. I also thought about maybe putting symbols next to them to mark them as 'In Progress'

Copy link
Member

Choose a reason for hiding this comment

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

Assuming the answer is yes (sorry, going to bed soon 😂 ), I don't think it is really clear (even if the answer is no I think it's not clear!). Also: I think it's easy to get outdated links once we close or deprecate issues. I think maybe for TODOs we can have a link at the bottom with a big global issue pointing to the current work, or maybe even to an issue search with label next-feature or something. Just my 2c!

@trivigy
Copy link
Member

trivigy commented Dec 12, 2020

Would not have changed anything for me. WebRTC is a fairly complex tech stack and usually requires decent know how. By the time I was using it full force I already knew what is inside.

P.s. but the extra docs are always very welcome!

Copy link
Contributor

@tarrencev tarrencev left a comment

Choose a reason for hiding this comment

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

much better!

README.md Outdated
* Plan-B and Unified Plan
* [SettingEngine](https://pkg.go.dev/github.com/pion/webrtc/v3#SettingEngine) for Pion specific extensions

#### Pure Go
Copy link
Contributor

Choose a reason for hiding this comment

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

minor feedback: this section in between the webrtc specific functionality seems a bit out of place

Copy link
Member Author

Choose a reason for hiding this comment

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

moved to the bottom!

@ashellunts
Copy link
Contributor

Definitely good improvement of README!

@jeremija
Copy link
Member

jeremija commented Dec 12, 2020

I like this!

My two cents: the pion/webrtc dependencies under the pion/ organization sometimes miss the details on what they are for. For example, when I was starting out exploring pion and its libraries, I noticed the READMEs of pion/rtp, pion/rtcp and pion/srtp all looked the same, with no additional context about what each of these libraries were about.

On the other hand, the pion/dtls library has a nice list with features at the top of the README.

@r-novel
Copy link
Member

r-novel commented Dec 12, 2020

Looks good to me!

@TannerGabriel
Copy link
Member

It seems like a great improvement!

@audiolion
Copy link

Love this change 🙌

@jech
Copy link
Member

jech commented Dec 12, 2020

With all due respect, Sean, I think the "feature matrix" is overselling Pion. It might raise people's expectations too much, and then cause them to turn away from Pion when they realise that we've been overselling.

I recommend marking Simulcast and SVC as "work in progress". I don't know what is the status of mediadevices, please mark it as experimental or something. Please be clear upfront that the user must bring their own implementations of congestion control and loss recovery.

Copy link
Member

@lherman-cs lherman-cs left a comment

Choose a reason for hiding this comment

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

It looks really good!

@Sean-Der Sean-Der force-pushed the issue-1279 branch 3 times, most recently from fe9f788 to 0ec266f Compare December 12, 2020 23:46
@backkem
Copy link
Member

backkem commented Dec 13, 2020

I like this!

My two cents: the pion/webrtc dependencies under the pion/ organization sometimes miss the details on what they are for. For example, when I was starting out exploring pion and its libraries, I noticed the READMEs of pion/rtp, pion/rtcp and pion/srtp all looked the same, with no additional context about what each of these libraries were about.

On the other hand, the pion/dtls library has a nice list with features at the top of the README.

We split every protocol out to encourage them to be used on their own. Some didn't get much attention while others, like DTLS, grew their own vibrant community. It just happened naturally. Maybe improving the readmes of those other libraries may help attract eyes thought.

Copy link
Member

@steebchen steebchen left a comment

Choose a reason for hiding this comment

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

LGTM; when I started with WebRTC I've had trouble what this repository exactly is and how it relates to all the others. It's pretty clear with this PR. 👍

@tekig
Copy link
Member

tekig commented Dec 13, 2020

Many complex terms can scare people off or raise their expectations. They may say "A lot has been done here, we can easily use this project", but they are not.

I think that people who understand will not switch to another project, because they already have a ready-made solution that works. Not sure, but people come here who don't know much about WebRTC in the beginning. The words ICE, TURN and etc wouldn't give me anything. I wanted to see an example that works and is simple so that a quick start can be done.

I think at the beginning you need to put a quick start, only then the features. An example should implement the most popular usecase, and also offer examples for less popular usecase. These examples should work in the real world so that the user does not think they have been fooled.

Sorry a little off topic.

@Sean-Der
Copy link
Member Author

@tekig what do you think of pushing features down lower? We can have Usage and What is WebRTC and then features.

I think we do have some people coming to this and evaluating how polished Pion is (where I think this list helps). I think you are right though about the majority of users don't care about feature set and just want to get going.

@thinkski
Copy link
Member

Would this have improved/detracted from your first opinion of using Pion? I have gotten a lot of feedback that people have no idea what pion/webrtc contains so wanted to bring this to the top.

Having this list of features is great, although the README is starting to get busy -- maybe better to have a 'Features' page on the wiki or pion.ly and link to it from the README.md?

Personally, I learn best by example, so the example applications/demos were the most useful to get a feel for Pion.

@tekig
Copy link
Member

tekig commented Dec 14, 2020

@tekig what do you think of pushing features down lower? We can have Usage and What is WebRTC and then features.

a good idea

List most popular features/benefits of Pion WebRTC.

Resolves #1279

Co-authored-by: ZHENK <[email protected]>
@Sean-Der Sean-Der merged commit f5875d9 into master Dec 14, 2020
@Sean-Der Sean-Der deleted the issue-1279 branch December 14, 2020 18:15
@wdouglass
Copy link
Member

I'm late on this, but it's a good change!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Include Features Matrix