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.

Sign up for GitHub

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

Jump to bottom

Rtmp client #2503

Merged

ojw28 merged 2 commits into google:dev-v2 from b95505017:rtmp_client

Jul 5, 2017

Contributor

b95505017 commented Feb 26, 2017 •

edited

Loading

RTMP DataSource extension

googlebot commented Feb 26, 2017

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

googlebot added the cla: no label

ojw28 reviewed

View reviewed changes

Contributor

ojw28 left a comment

Thanks for the pull request. I think it needs splitting in two (one request for the FLV changes, and another for the RTMP feature). Please also see inline comments. Thanks!

demo/build.gradle Outdated

@@ @@ -45,4 +45,5 @@ dependencies { @@
                   withExtensionsCompile project(path: ':extension-flac')
                   withExtensionsCompile project(path: ':extension-opus')
                   withExtensionsCompile project(path: ':extension-vp9')
+                  compile 'net.butterflytv.utils:rtmp-client:0.2.6.1'

Contributor

ojw28 Feb 26, 2017

We don't want this dependency from the demo app. For all other cases where we've taken an external dependency, we've done so by creating an extension (see the six existing extensions here).

If you restructure this change so that RtmpDataSource is in a proper extension then we'll consider accepting a pull request. We probably don't want any samples in the demo app by default.

demo/src/main/java/com/google/android/exoplayer2/demo/player/RtmpDataSource.java Outdated

		@@ -0,0 +1,55 @@
		package com.google.android.exoplayer2.demo.player;

Contributor

ojw28 Feb 26, 2017

You need to put the standard header present in other ExoPlayer source files at the top here

demo/src/main/java/com/google/android/exoplayer2/demo/player/RtmpDataSource.java Outdated

+              import java.io.IOException;
+              /**
+               * Created by faraklit on 08.01.2016.

Contributor

ojw28 Feb 26, 2017

Needs proper Javadoc

demo/src/main/java/com/google/android/exoplayer2/demo/player/RtmpDataSource.java Outdated

		public class RtmpDataSource implements DataSource {


		private final RtmpClient rtmpClient;

Contributor

ojw28 Feb 26, 2017

Please use 2 character indentation as in other ExoPlayer source files

demo/src/main/java/com/google/android/exoplayer2/demo/player/RtmpDataSource.java Outdated

+                  @Override
+                  public Uri getUri() {
+                      return uri;

Contributor

ojw28 Feb 26, 2017

Hm. This is always null? I think it needs setting in open and clearing in close?

library/src/main/java/com/google/android/exoplayer2/extractor/flv/ScriptTagPayloadReader.java Outdated

-                      durationUs = (long) (durationSeconds * C.MICROS_PER_SECOND);
+              //    if (type != AMF_TYPE_ECMA_ARRAY) {
+              //      //   Should never happen.
+              //      //aom: actually it can happen for live streams

Contributor

ojw28 Feb 26, 2017

Please delete the code block if that's the case

library/src/main/java/com/google/android/exoplayer2/extractor/flv/ScriptTagPayloadReader.java Outdated

                   }
                 }
+                public void setKeyFrameTimes(List<Double> keyFrameTimes) {

Contributor

ojw28 Feb 26, 2017

I'm unclear why you need these setter methods. Why can't you just assign the variables directly?

library/src/main/java/com/google/android/exoplayer2/extractor/flv/ScriptTagPayloadReader.java Outdated

-                  int count = data.readUnsignedIntToInt();
-                  HashMap<String, Object> array = new HashMap<>(count);
-                  for (int i = 0; i < count; i++) {
+                  //ecma array is not the strict array, so it shouldn't be trusted number of item count

Contributor

ojw28 Feb 26, 2017

Rather than this comment, it would probably be clearer to do:
int approximateCount = data.readUnsignedIntToInt();

It's probably still worth passing the value to the HashMap constructor as an initial size as well, as was previously the case.

library/src/main/java/com/google/android/exoplayer2/extractor/flv/ScriptTagPayloadReader.java Outdated

+                  this.keyFrameFilePositions = keyFrameFilePositions;
+                }
+                public List<Double> getKeyFrameFilePositions() {

Contributor

ojw28 Feb 26, 2017

It would be nice if these getters were moved to be next to the getDurationUs.

library/src/main/java/com/google/android/exoplayer2/extractor/flv/ScriptTagPayloadReader.java Outdated

+                    }
+                    if (metadata.containsKey(KEY_FRAMES)) {
+                      HashMap<String, List<Double>> keyFrames = (HashMap<String, List<Double>>) metadata.get(KEY_FRAMES);
+                      setKeyFrameTimes(keyFrames.get(KEY_FRAME_TIMES));

Contributor

ojw28 Feb 26, 2017

It might be nice to convert the values into long microsecond and byte offset arrays here, rather than keeping them as doubles. That way the public getter methods, for time at least, are all in the same unit (i.e. microseconds).

Contributor

ojw28 commented Mar 15, 2017

@b95505017 - Do you have any plans to split this request, address the comments and sign the CLA? Else I'll go ahead and close this. There's definitely nothing we can do with it without the CLA being signed, and I'd rather not leave it open indefinitely.

Contributor Author

b95505017 commented Mar 15, 2017

I've signed the CLA already, but @mekya. He is the main author about those commits, I just made them rebase v2.

mekya commented Mar 16, 2017

I have signed CLA and I am going to try to fix the issues in your( @ojw28 ) comments in a week. I am sorry I am busy in these days. Btw, Thank you @b95505017

tylerjroach mentioned this pull request

Added mp3 support to FLV extractor #2683

Merged

ojw28 mentioned this pull request

Add RTMP support #2590

Closed

b95505017 force-pushed the rtmp_client branch from 22f3b76 to 9db7e29 Compare

April 30, 2017 00:57

googlebot commented Apr 30, 2017

CLAs look good, thanks!

googlebot added cla: yes and removed cla: no labels


          Rtmp extension

4343ce6

b95505017 force-pushed the rtmp_client branch from 9db7e29 to 4343ce6 Compare

April 30, 2017 00:59

Contributor Author

b95505017 commented Apr 30, 2017

@ojw28 I refactor everything, please take a look again thanks!

ojw28 mentioned this pull request

any plan to support Exo 2.0 ant-media/LibRtmp-Client-for-Android#17

Closed

Contributor

ojw28 commented May 2, 2017 •

edited

Loading

This is looking much better now; thanks! I have some concerns about the underlying library being used. I've filed a couple of issues on that library directly. If you're able to help with, or comment on, those issues directly, that might help speed things up! The issues are:

ant-media/LibRtmp-Client-for-Android#32
ant-media/LibRtmp-Client-for-Android#31
ant-media/LibRtmp-Client-for-Android#30

Contributor Author

b95505017 commented May 2, 2017

Actually the best choice of rtmp client is using the pure java Socket version instead of librtmp I think, but the concept will be based on SimpleRtmp which may hit the copyright. The pure java Socket version will be more extendable and have more control about the rtmp behavior including redirect behavior.

Not sure what is the best choice for ExoPlayer project.

Contributor

ojw28 commented May 2, 2017

If there's a pure Java RTMP library available somewhere under a sensible license, that's distributed via jcenter, then it's fine to depend on that instead. I'd rather not implement the whole thing inside the extension if possible; having a dependency is preferable. If the whole thing is implemented inside the extension then it needs to be original work, not copy/paste or derivative of some other project, as per the CLA.

Contributor

ojw28 commented May 3, 2017 •

edited

Loading

I got this working reasonably well, but I found another issue (ant-media/LibRtmp-Client-for-Android#33). I think we'll probably need the issues we've raised to be fixed in LibRtmp before we're able to merge this. Alternatively, we could depend on a different project if there's a better alternative.

Contributor Author

b95505017 commented May 3, 2017

That's weird, they claim have all ABIs.

It is probably the smallest(~60KB) rtmp client for android. It calls librtmp functions over JNI interface. With all cpu architectures(arm, arm7, arm8, x86, x86-64, mips) its size is getting about 300KB

mekya commented May 3, 2017

@b95505017 @ojw28
This librtmp java wrapper library is developed with a specific project that does not use any 64 bit library so it works on both 32-bit and 64-bit devices. In order to support all cpu arch., we just need to add only a configuration parameter when building.

In addition, before I develop this library, I googled a RTMP Java library. I found some libraries however they were complex and their sizes are a few MBs.

ojw28 mentioned this pull request

Include 64-bit libraries in releases ant-media/LibRtmp-Client-for-Android#33

Closed

Contributor

ojw28 commented May 8, 2017

@mekya - Thanks for the improvements so far! There are a couple of remaining issues (mostly minor things at this point, but now might be a good time to make those tweaks if you agree they're good ideas). Once we have those + a release with the 64bit variants, I think we should be good to go ahead and merge an updated version of this change.

mekya commented May 9, 2017

@ojw28 yeah I agree with you, I think it is the right time to make these minor changes as well.
I am gonna try to fix ant-media/LibRtmp-Client-for-Android#34 and ant-media/LibRtmp-Client-for-Android#31

is it ok? Any other issue?

Bests,
A. Oguz

Contributor

ojw28 commented May 9, 2017

I don't think I have any other suggestions, no. Thanks!

Contributor

ojw28 commented May 24, 2017

@mekya - Do you have any kind of time estimate for the remaining changes + an updated release? No worries if not; just curious :).

Contributor

ojw28 commented Jun 13, 2017

Ping @mekya :).

mekya commented Jun 13, 2017

64 bytes from @mekya icmp_seq=0 ttl=30 days time=20 days :P

I am sorry for late reply @ojw28 , please check the latest commits in the repo

Contributor

ojw28 commented Jul 5, 2017

We still have one open issue on LibRtmp here, which is affecting the minimum sdk version we can set for the extension to 21 (not particularly useful). Tracked by ant-media/LibRtmp-Client-for-Android#39. That said, I think we can merge + cleanup at this point.


          Merge branch 'dev-v2' into rtmp_client

0ecbe5d

ojw28 merged commit 1279b7d into google:dev-v2

ojw28 added the should merge label

google locked and limited conversation to collaborators

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cla: yes should merge