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 RTMP support #2590

Closed
wants to merge 4 commits into from
Closed

Add RTMP support #2590

wants to merge 4 commits into from

Conversation

begeekmyfriend
Copy link

@begeekmyfriend begeekmyfriend commented Mar 22, 2017

I have implemented RTMP in pure Java for ExoPlayer with the HKS-TV RTMP live stream link added into media.exolist.json as a demo for watching.

The commits include:

  • RTMP implementation modified from SimpleRTMP.
    • Amf0 data format;
    • RTMP packet header format;
    • RTMP chunk stream format;
    • RTMP packet decoder;
    • RTMP connection and session.
  • RTMP data source porting which is consistent with other upstream protocol such as HTTP.
  • FLV stream extractor adaptation for RTMP to parse FLV stream within RTMP packets.
  • RTMP uri parser.

My PR works well under HKS-TV RTMP live stream link.

This commit includes RTMP stream pull implementation and data source porting.

The implementation of RTMP stream pull includes:

- Amf0 data format;
- RTMP packet header format;
- RTMP chunk stream format;
- RTMP packet decoder;
- RTMP connection and session.

And the data source API is consistent with other stream protocol such as
HTTP and so on.

Signed-off-by: Leo Ma <[email protected]>
Add FLV stream extractor to parse FLV stream within RTMP packets. This
extractor is similar to FLV extractor except for some FLV tag headers.
So I separate it as a single file. We can consider to merge it into FLV
extractor in the future.

Signed-off-by: Leo Ma <[email protected]>
Add HKS-TV RTMP link under Misc directory of media.exolist.json to
demonstrate RTMP play.

Note there is no special extensions in RTMP uri so I have to modify
the URI parser to make it work.

Signed-off-by: Leo Ma <[email protected]>
Note this modification is trial since I have no other way to recover
RTMP playing after disabling both AV tracks/renderers on UI. I find it
impossible to switch to the BUFFERING state under the ENDED state when
playing RTMP live stream. Maybe we need to redefine the state machine to
control both AV tracks/renders under RTMP.

Signed-off-by: Leo Ma <[email protected]>
@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@begeekmyfriend
Copy link
Author

I signed it!

@googlebot
Copy link

CLAs look good, thanks!

@begeekmyfriend
Copy link
Author

Any response?

@ojw28
Copy link
Contributor

ojw28 commented Mar 27, 2017

This change touches 45 files. It's going to take quite a long time for us to look at it. In the meantime, you could helpfully re-send it to the correct branch (dev-v2, not release-v2). Thanks!

@begeekmyfriend
Copy link
Author

Thanks for response. The branch that the PR send to is really dev-v2.

The number of changed files is 45 but only 6 of them are modified with few lines of code from the original ones. So I think it has little influence on the original feature.

@ojw28
Copy link
Contributor

ojw28 commented Mar 27, 2017

Ah yes, ignore comment about branch, sorry!

@tylerjroach
Copy link
Contributor

@begeekmyfriend It looks like dev-v2 just went through a huge file structure change. I've taken the time to try and split up your commits in the direction it looks like they are going. Feel free to use that as a reference when updating your pull request. https://github.com/tylerjroach/ExoPlayer/commits/rtmp

We've been using Exoplayer with RTMP in production using ndk + librtmp for over a year now and I have a few fixes to add:
*Out of Memory crashes caused in FlvExtractor and VideoTagPayloadReader
*Mp3 support #2683
*Support for video only streams
*Support for audio only streams

I'll commit them to my rtmp branch tomorrow, but let me know if you want me to work on your branch, that way we can get all of the fixes in with this pull request. It would also be helpful to hear from @ojw28 on which direction he is leaning for rtmp support. I'm inclined to vote for this one as it's a pure java solution. Thanks and nice work!

@linyongsheng
Copy link

@tylerjroach I use Exoplayer to play FLV live stream and get more OOM .How to fix it? Thanks!

@begeekmyfriend
Copy link
Author

@tylerjroach Of course I am glad to see that my work would be paid attention and more people are willing to join working on this branch!

@tylerjroach
Copy link
Contributor

@linyongsheng I'll try to submit PR's to fix the OOM issues to dev-v2 today. I'll also work on creating a more stable rtmp playback branch based off this PR. @begeekmyfriend What are your thoughts on rebasing this request on the new structure of dev-v2. This way we don't end up with a ugly list of commits. If you can take care of that, I'll start adding fixes on top of your branch. Thanks!

@begeekmyfriend
Copy link
Author

@tylerjroach The conflicting files seem not key files for the RTMP module. The data source and extractor are both independent with other formats. I do not know which files would you like to update

@tylerjroach
Copy link
Contributor

@begeekmyfriend Its more of the structure of the project. See the latest dev-v2 here.
https://github.com/google/ExoPlayer/tree/dev-v2/library

The libarary code has now been split into different folders. Ex core, hls. Presumably this is probably where we should add rtmp.

dev-v2/library/src is no longer the folder structure for any library code.

@begeekmyfriend
Copy link
Author

@tylerjroach I think this file structure change just illustrate that the maintainers are considering add RTMP as a module like HLS, DASH and so on beside the core module. What we can do is better wait and see what they would do in the future.

@tylerjroach
Copy link
Contributor

Yeah, I'm not sure how stable dev-v2 structure is at this point, but I'm sure this PR wont be reviewed until the conflicts have been resolved and we are using the correct structure. I can send a PR on your branch that updates the structure of the project and fixes the conflicts.

@tylerjroach
Copy link
Contributor

@begeekmyfriend I added a pull request to your repo to get fix the conflicts and attempt to match the file structure of the current dev-v2 project: begeekmyfriend#1

@linyongsheng You can check out my most up to date repo of ExoPlayer with RTMP support here:
https://github.com/tylerjroach/ExoPlayer/tree/dev-v2-rtmp-latest

This branch contains everything from @begeekmyfriend as well as:
*mp3 audio support added
*2 OOM fixes that could occur in some situations

I will be adding the ability to play audio and video only RTMP streams to that branch shortly.

@linyongsheng
Copy link

@tylerjroach I find ExoPlayerImplInternal class in " https://github.com/tylerjroach/ExoPlayer/tree/dev-v2-rtmp-latest" has some different。Line 514~515 :
// setState(ExoPlayer.STATE_ENDED);
// stopRenderers();
why these two lines has commented?

@begeekmyfriend
Copy link
Author

@linyongsheng It is a trivial commit 9ab24c9

@winlinvip
Copy link

winlinvip commented Apr 22, 2017

I'm the owner of SRS, a realtime live streaming server, delivery RTMP/HTTP-FLV/HLS. For RTMP/HTTP-FLV, it's about 1-3s latency; while it's 10s+ for HLS. So, it's very important for player to support RTMP/HTTP-FLV, and I know that the great ExoPlayer has already support HTTP-FLV, it's awesome when ExoPlayer also support RTMP, because there are lots of encoder/server/CDN only support RTMP but not support HTTP-FLV. It seems that facebook also use RTMP.

In the past years, live streaming industry is all about PC/Flash, the latency for a live streaming system can be around 1s, to use FMLE/FFMPEG/Flash as encoder(to encode YUV/PCM to H.264/AAC), then delivery by RTMP server such as FMS/AMS/WOWZA/NGINX-RTMP/SRS, finally pull to Flash player over RTMP. Now, we're in mobile world, but the live streaming is still using HLS, because both Android/iOS don't support RTMP/HTTP-FLV, it's really a hard time for mobile APP company to develop a realtime live streaming product.

Thanks Google, thanks ExoPlayer, atleast the Android solution for 1s live streaming is possible, by using bellow open-source products:

  1. Encoder: YASEA and Compare SDKs
  2. Server: SRS
  3. Player: ExoPlayer and Compare SDSs.

Thanks~

@ojw28
Copy link
Contributor

ojw28 commented Apr 22, 2017

Regarding the best direction for adding RTMP support:

  • I don't think it matters too much whether it's a pure Java solution or depends on a native library, so long as any native library part doesn't contain things that people are likely to need to customize or extend.
  • It should probably be a separate module if part of the core library (e.g. library-rtmp), or an extension (e.g. extension-rtmp). The difference between a library module and an extension module is that extension modules depend on external libraries to provide their functionality, where-as library modules implement it themselves.

Regarding this change specifically:

  • It appears to include code taken from an LGPL licensed project. We cannot accept contributions that do this. Nor can we accept contributions that include derivative works of such projects. The CLA requires that all contributions are your original creations. So options are (a) implement RTMP from scratch yourself, or (b) follow the extension model, with the extension module having a dependency on an external RTMP library. I would have thought (b) is probably the better approach, since (a) is going to involve re-inventing the wheel, and moves the maintenance burden of the RTMP implementation to the ExoPlayer project maintainers. Pull request Rtmp client #2503 used net.butterflytv.utils:rtmp-client and looked promising, but the contributor hasn't yet refactored it to be a proper extension. If someone were to do that then we'd definitely take a serious look at merging such an extension.

@ojw28 ojw28 added cla: no and removed cla: yes labels Apr 22, 2017
@winlinvip
Copy link

Encryption is not need for RTMP, client can use simple handshake with RTMP server. The complex handshake(requires encryption) is only necessary at server-side to serve flash client.

@begeekmyfriend
Copy link
Author

begeekmyfriend commented Apr 25, 2017

@winlinvip Thank you for your advice all the same! The handshake invoked in RTMP connection is still simple but not complex, and therefore in fact, the encryption functions are not used in the code.

@ojw28
Copy link
Contributor

ojw28 commented May 2, 2017

I'm going to close this due to the licensing issues mentioned above, and because #2503 is looking like a more promising approach. Let's try and get that one merged instead.

@ojw28 ojw28 closed this May 2, 2017
@google google locked and limited conversation to collaborators Aug 31, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants