Media streaming with NC14 endpoint #2524

tobiasKaminsky · 2018-04-27T08:19:40Z

Needs:

Fixes: #379

This enables video/audio streaming without exposing any credentials.

Test cases:
NC14

non downloaded file
- stream video internally
  - toggle to fullscreen
- stream audio internally
- stream video with external app
- stream audio with external app
downloaded file
- play video/audio directly

NC13

no option to stream
non downloaded file
- download media, then play
downloaded file
- play video/audio directly

Signed-off-by: tobiasKaminsky tobias@kaminsky.me

tobiasKaminsky · 2018-04-27T08:41:00Z

For such PR that need a library PR, I guess the best is to reference the branch directly, or?
Problem will be, when we merge the corresponding PR, then there is no PR and the build fails. Then we would have to use master until we release a new library version, otherwise any other client PR will fail.

--> I am not sure how to handle this best…

AndyScherzinger · 2018-04-27T09:33:14Z

src/main/java/com/owncloud/android/files/StreamMediaFileOperation.java

+                // Parse the response
+                JSONObject respJSON = new JSONObject(response);
+                String url = (String) respJSON.getJSONObject(NODE_OCS).getJSONObject(NODE_DATA).get(NODE_URL);
+


couldn't you just use getString instead of get()+cast ?

Indeed 👍

mario · 2018-04-27T09:49:03Z

Shouldn't stream just be the default instead of having a dedicated stream menu button? What does "stream" means to say, my grandma?

AndyScherzinger · 2018-04-27T09:57:38Z

src/main/java/com/owncloud/android/ui/helpers/FileOperationsHelper.java

+                mFileActivity.startActivity(Intent.createChooser(openFileWithIntent,
+                        mFileActivity.getString(R.string.stream)));
+            }
+        }).start();


optional: What about using a lambda and putting the whole run() code into a separate method?

Yup please.

AndyScherzinger · 2018-04-27T09:58:44Z

src/main/java/com/owncloud/android/ui/preview/PreviewMediaFragment.java

+
+                new LoadStreamUrl(this, client).execute(getFile().getLocalId());
+            } catch (Exception e) {
+                Log_OC.e(TAG, "Loading stream url not possible: " + e.getMessage());


shouldn't we also add the exception itself to the log statement?

AndyScherzinger · 2018-04-27T09:59:05Z

src/main/java/com/owncloud/android/ui/preview/PreviewMediaFragment.java

+                            R.string.stream_not_possible_message, R.drawable.file_movie);
+                }
+            } else {
+                Log_OC.e(TAG, "Error streaming file: no previewMediaFragment!");


Same here: shouldn't we also add the exception itself to the log statement?

Here is no exception.
It happens when fragment is null.

AndyScherzinger · 2018-04-27T10:01:32Z

Just some minor code comments

Shouldn't stream just be the default instead of having a dedicated stream menu button?

I agree it should be the default cation when clicking on an item (that can be streamed in general) and I remember that this is also what Tobias told me :)

tobiasKaminsky · 2018-04-27T10:12:50Z

It is: if you click on audio/video it will try to stream the file with our internal player.
You can also open the overflow menu and select "stream with…" to directly open up the file in e.g. vlc.

If our internal player fails, it says "Please download media instead or use external app.".

mario · 2018-05-22T08:46:55Z

src/main/java/com/owncloud/android/media/MediaService.java

        return mMediaController;
    }

+    private static class LoadStreamUrl extends AsyncTask<String, Void, String> {


Is there no other way but an AsyncTask? I thought we got rid of this ages ago :P

mario · 2018-05-22T09:00:27Z

To further clarify my comment on AsyncTasks:

TobiasK: as for media streaming: I'd appreciate if we'd move away from asynctasks, especially for new code
even a service-based download + eventbus/interface download notification would work better
but I guess you could go for a Job + eventbus notification too, which would make it cleeeeeaner :)

tobiasKaminsky · 2018-05-22T09:19:24Z

For up/downloading I agree to use services or jobs/eventbus.
But for this we need to have this executed as soon as possible as it fetches the video streaming url.

So I would go for this:

if it "urgent"/ as soon as possible, use asyncTask (afaik it is still the recommended way of android to show results of small background options on the UI).
all other, especially things that are really asynchronous, like upload/download, keep-up-to-date should be done via jobs, so that android doze and other battery optimizations can take care of it

mario · 2018-05-22T09:22:35Z

Ah. Service or Job can be ran right away too, but I just noticed that it's a simple fetching of url, not of an actual image.

Regardless, shouldn't we do it differently in a way that survives rotations etc instead of having to worry about these things?

rullzer · 2018-06-27T07:14:09Z

Would be great to get this in. NC14 will go into freeze soon. So having a client that uses it will allow us to properly test it.

tobiasKaminsky · 2018-06-27T10:00:14Z

Rebased & ready for final review

AndyScherzinger · 2018-06-27T10:09:56Z

👍 fine by me :)

codecov · 2018-06-27T11:37:31Z

Codecov Report

Merging #2524 into master will increase coverage by 0.02%.
The diff coverage is 0%.

@@            Coverage Diff            @@
##           master   #2524      +/-   ##
=========================================
+ Coverage     6.4%   6.42%   +0.02%     
=========================================
  Files         294     297       +3     
  Lines       29450   29709     +259     
  Branches     4252    4284      +32     
=========================================
+ Hits         1885    1910      +25     
- Misses      27278   27515     +237     
+ Partials      287     284       -3

Impacted Files	Coverage Δ
...cloud/android/ui/helpers/FileOperationsHelper.java	`0% <0%> (ø)`	⬆️
...cloud/android/ui/preview/PreviewVideoActivity.java	`0% <0%> (ø)`	⬆️
.../java/com/owncloud/android/media/MediaService.java	`0% <0%> (ø)`	⬆️
...cloud/android/ui/preview/PreviewMediaFragment.java	`0% <0%> (ø)`	⬆️
...cloud/android/ui/activity/FileDisplayActivity.java	`0% <0%> (ø)`	⬆️
...ava/com/owncloud/android/files/FileMenuFilter.java	`0% <0%> (ø)`	⬆️
...ncloud/android/files/StreamMediaFileOperation.java	`0% <0%> (ø)`
...ncloud/android/ui/fragment/OCFileListFragment.java	`0% <0%> (ø)`	⬆️
...ncloud/android/datamodel/SyncedFolderProvider.java	`8.8% <0%> (-1.3%)`	⬇️
.../owncloud/android/ui/activity/ToolbarActivity.java	`0% <0%> (ø)`	⬆️
... and 18 more

AndyScherzinger · 2018-06-28T06:35:19Z

@tobiasKaminsky what about @mario's latest comment: #2524 (comment) ?

tobiasKaminsky · 2018-06-28T08:17:45Z

Both loading url task are done when playing media, so I think it is fine.
Android Dev guide also recommends async task for short running calls.

Beside that, there is somewhere an issue, where I started a discussion how to do long running background tasks, that maybe even survive activity rotate/open/close.

--> I would keep it this way and later replace it with a potential better solution.

AndyScherzinger · 2018-06-28T08:21:13Z

--> I would keep it this way and later replace it with a potential better solution.

Fine with me

tobiasKaminsky · 2018-06-28T08:23:48Z

E.g. UserInfoActivity is the same:
open an account -> fetches data
rotate screen -> fetches data again

Of course, doing the same is not pushing the code to a better state, but I think we should find a better and generic approach first and then change it.

mario · 2018-07-12T12:27:32Z

👍

Signed-off-by: tobiasKaminsky <tobias@kaminsky.me>

nextcloud-android-bot · 2018-07-13T07:39:47Z

Lint

This	93 warnings
Master	93 warnings

FindBugs (new)

Warning Type	Number
Bad practice Warnings	35
Correctness Warnings	166
Experimental Warnings	4
Internationalization Warnings	12
Malicious code vulnerability Warnings	9
Multithreaded correctness Warnings	9
Performance Warnings	158
Security Warnings	163
Dodgy code Warnings	207
Total	763

FindBugs (master)

Warning Type	Number
Bad practice Warnings	35
Correctness Warnings	166
Experimental Warnings	4
Internationalization Warnings	12
Malicious code vulnerability Warnings	9
Multithreaded correctness Warnings	9
Performance Warnings	158
Security Warnings	163
Dodgy code Warnings	207
Total	763

tobiasKaminsky added the 3. to review label Apr 27, 2018

AndyScherzinger reviewed Apr 27, 2018

View reviewed changes

nextcloud deleted a comment Apr 27, 2018

AndyScherzinger reviewed Apr 27, 2018

View reviewed changes

mario reviewed May 22, 2018

View reviewed changes

tobiasKaminsky force-pushed the videoStreaming branch from 11c45f0 to 88fd930 Compare June 27, 2018 10:00

nextcloud deleted a comment Jul 3, 2018

tobiasKaminsky added 5 commits July 12, 2018 18:20

media streaming with NC14 endpoint

b8bceee

Signed-off-by: tobiasKaminsky <tobias@kaminsky.me>

remove cast

f703911

Signed-off-by: tobiasKaminsky <tobias@kaminsky.me>

Changes due to CR

5b1a011

Signed-off-by: tobiasKaminsky <tobias@kaminsky.me>

changes due to rebase

e11f00a

Signed-off-by: tobiasKaminsky <tobias@kaminsky.me>

remove unneeded string

8869e4c

Signed-off-by: tobiasKaminsky <tobias@kaminsky.me>

AndyScherzinger force-pushed the videoStreaming branch from 46a3694 to 8869e4c Compare July 12, 2018 16:20

nextcloud deleted a comment Jul 12, 2018

Changes due to Codacy

c29ccf6

Signed-off-by: tobiasKaminsky <tobias@kaminsky.me>

nextcloud deleted a comment Jul 13, 2018

tobiasKaminsky merged commit cdd3a56 into master Jul 13, 2018

tobiasKaminsky deleted the videoStreaming branch July 13, 2018 07:54

AndyScherzinger added this to the Nextcloud App 3.3.0 milestone Jul 13, 2018

AndyScherzinger mentioned this pull request Aug 8, 2018

Don't keep a downloaded copy when opened #2783

Closed

AndyScherzinger removed the 3. to review label Aug 29, 2018

Uh oh!

Media streaming with NC14 endpoint #2524

Media streaming with NC14 endpoint #2524

Uh oh!

Conversation

tobiasKaminsky commented Apr 27, 2018 • edited by mario Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tobiasKaminsky commented Apr 27, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mario commented Apr 27, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tobiasKaminsky Jun 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AndyScherzinger commented Apr 27, 2018

Uh oh!

tobiasKaminsky commented Apr 27, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mario commented May 22, 2018

Uh oh!

tobiasKaminsky commented May 22, 2018

Uh oh!

mario commented May 22, 2018

Uh oh!

rullzer commented Jun 27, 2018

Uh oh!

tobiasKaminsky commented Jun 27, 2018

Uh oh!

AndyScherzinger commented Jun 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Jun 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

AndyScherzinger commented Jun 28, 2018

Uh oh!

tobiasKaminsky commented Jun 28, 2018

Uh oh!

AndyScherzinger commented Jun 28, 2018

Uh oh!

tobiasKaminsky commented Jun 28, 2018

Uh oh!

mario commented Jul 12, 2018 • edited by AndyScherzinger Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nextcloud-android-bot commented Jul 13, 2018

Lint

FindBugs (new)

FindBugs (master)

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

tobiasKaminsky commented Apr 27, 2018 •

edited by mario

Loading

tobiasKaminsky Jun 27, 2018 •

edited

Loading

AndyScherzinger commented Jun 27, 2018 •

edited

Loading

codecov bot commented Jun 27, 2018 •

edited

Loading

mario commented Jul 12, 2018 •

edited by AndyScherzinger

Loading