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

Postprocessing for downloads and implement missing features #1759

Merged
merged 15 commits into from Dec 11, 2018

Conversation

Projects
None yet
6 participants
@kapodamy
Copy link
Contributor

kapodamy commented Sep 23, 2018

This PR include:

  • Post-processing infrastructure
  • Proper error handling "infrastructure"
  • Queue instead of multiple downloads
  • Move serialized pending downloads (.giga files) to app data
  • Implement max download retry
  • Proper multi-thread download pausing
  • Stop downloads when swicthing to mobile network (never works, see 2nd point)
  • Save the thread count for next downloads
  • A lot of incoherences fixed

Also this include:

  • Mp4 DASH reader/writter
  • WebM reader/writter
  • a subtitle converter for Timed Text Markup Language v1 and TranScript (v1, v2 and v3)
  • SharpStream to wrap IntputStream and OutputStream in one interface
  • custom implementation of DataInputStream

More details:

  • Remove interfaces with one implementation
  • Fix download resources with unknown length
  • Marquee style for ProgressDrawable
  • "view details" option in mission context menu
  • Notification for finished downloads
  • Post-processing infrastructure: sub-missions, circular file, layers for layers of abstractions for Java IO streams
  • Mp4 muxing (only DASH brand)
  • WebM muxing
  • Captions downloading
  • Alert the user when trying to use a filename that is already in use in existing downloads (finished or not).
  • Major UI changes: divide downloads by pending and finished, crickets, advanced marquee.

Notes:

  • Unfinished downloads will be erased (new serialVersionUID) with this PR
  • Download database version bumped to 3
  • Snackbar notification for undo delete is hardcoded
  • CircularFile is like a circular buffer but in a file. This prevent use extra storage space
  • CircularFile can slowdown the app while post-processing long files (upper to 256MiB) a few seconds due heap resize
  • In the DonwloadDialog, video-only streams the character ~ will be shown in front of the size, in the future it should be shown to add the size of the audio stream

screenshot_1537727285 screenshot_1540512108
screenshot_1537727376screenshot_1540512051
screenshot_1543454560 screenshot_1543454573
screenshot_1540514866 screenshot_1540514841
screenshot_1540514805 screenshot_1540514912

@kapodamy kapodamy changed the title Giga postprocessing Giga postprocessing infrastructure Sep 23, 2018

@theScrabi

This comment has been minimized.

Copy link
Member

theScrabi commented Sep 23, 2018

Aaaaa I need help reviewing code O.o
@kapodamy please check why CI fails.

@kapodamy

This comment has been minimized.

Copy link
Contributor Author

kapodamy commented Sep 24, 2018

@theScrabi requires lastest NewPipeExtractor and TeamNewPipe/NewPipeExtractor#118

@theScrabi
Copy link
Member

theScrabi left a comment

This is huge code. I see the advantage of it, but it looks like a massive amount of maintenance.
The thing is we wanted to get rid of gigaget, however if we create more and more functionality around it it will become almost impossible to do that.

From a functional point however this is a cool change :)

@kapodamy

This comment has been minimized.

Copy link
Contributor Author

kapodamy commented Sep 26, 2018

most of the code moved to TeamNewPipe/NewPipeExtractor#119

@theScrabi theScrabi referenced this pull request Sep 28, 2018

Closed

HD videos with audio? #1755

@TobiGr TobiGr referenced this pull request Oct 4, 2018

Closed

Complaint #1766

@kapodamy kapodamy changed the title Giga postprocessing infrastructure Postprocessing for downloads and implement missing features Oct 25, 2018

@kapodamy

This comment has been minimized.

Copy link
Contributor Author

kapodamy commented Oct 26, 2018

Fixed: #1762 #1655 #1325 #602 (perhaps now)
Implemented: #1460 #1408 #1338 (no workaround) #1272 (thread count only) #961 #457 #355

kapodamy added some commits Sep 23, 2018

main commit
Post-processing infrastructure
* remove interfaces with one implementation
* fix download resources with unknow length
* marquee style for ProgressDrawable
* "view details" option in mission context menu
* notification for finished downloads
* postprocessing infrastructure: sub-missions, circular file, layers for layers of abstractions for Java IO streams
* Mp4 muxing (only DASH brand)
* WebM muxing
* Captions downloading
* alert dialog for overwrite existing downloads finished or not.

Misc changes
* delete SQLiteDownloadDataSource.java
* delete DownloadMissionSQLiteHelper.java
* implement Localization from #114

Misc fixes (this branch)
* restore old mission listeners variables. Prevents registered listeners get de-referenced on low-end devices
* DownloadManagerService.checkForRunningMission() now return false if the mission has a error.
* use Intent.FLAG_ACTIVITY_NEW_TASK when launching an activity from gigaget threads (apparently it is required in old versions of android)

More changes
* proper error handling "infrastructure"
* queue instead of multiple downloads
* move serialized pending downloads (.giga files) to app data
* stop downloads when swicthing to mobile network (never works, see 2nd point)
* save the thread count for next downloads
* a lot of incoherences fixed
* delete DownloadManagerTest.java (too many changes to keep this file updated)

kapodamy added some commits Nov 15, 2018

misc utils
Also this include:
* Mp4 DASH reader/writter
* WebM reader/writter
* a subtitle converter for Timed Text Markup Language v1 and TranScript (v1, v2 and v3)
* SharpStream to wrap IntputStream and OutputStream in one interface
* custom implementation of DataInputStream
@theScrabi

This comment has been minimized.

Copy link
Member

theScrabi commented Nov 17, 2018

Please make the pullrequest compile.
You can use the latest version of the extractor which contains your changes for SubtitlesStream: 91b1efc97e904a.

I tried compiling NewPipe against this commit, however there are still some other build issues. Pleas fix those before I can continue my review.

@kapodamy

This comment has been minimized.

Copy link
Contributor Author

kapodamy commented Nov 19, 2018

@theScrabi jitpack is providing an empty jar https://jitpack.io/#TeamNewPipe/NewPipeExtractor/91b1efc97e.
and only contains a manifest.
nvm is fixed

misc fixes
* use getPreferredLocalization() instead of getLocalization()
* use lastest commit in build.gradle
* fix missing cast in MissionAdapter.java
@TobiGr

This comment has been minimized.

Copy link
Member

TobiGr commented Nov 19, 2018

It is great to see this PR compiling again :) I did a small test and I have to admit that I love your changes. Let's see what we can improve to make this PR even better.

When a download is finished the notification informs the user about this. I think to make clear which app set up this notification, we should add NewPipe's logo to it. Additionally, we could enable line breaks whenever the title is too long.
download_finished_notification_icon

Using the light theme, the titles in the downloads list are hard to read.
download_queue_title_ligh_theme

After queuing a video, it shows an estimated size of 2MB which is clearly incorrect. Is it possible to pass the file size we fetched when showing the download dialog?
download_queue_estimated_file_size


EDIT: Clicking on Show info opens the video detail screen correctly. But pressing the hardware back button afterwards brings you back to the main screen.

@Riotous

This comment has been minimized.

Copy link

Riotous commented Nov 20, 2018

Finally, the PR so many of us have been waiting for!

@TobiGr

This comment has been minimized.

Copy link
Member

TobiGr commented Nov 20, 2018

I started 2 downloads and tested the queuing behaviour (started and stopped the downloads twice). Everything was fine and the downloads continued. After a while the screen went off and I did not pay attention to the downloads anymore. After two or three minutes I realized the downloads stopped with following errors:

download_error_2
download_error_1

Unfortunately, I was not able to restart the download.

EDIT: For some reason, NewPipe cleared the download history after closing the app and does not show these failed downloads anymore. Unfortunately, the download files still exist and cannot be deleted from within the app.


The downloads view crashes when rotating the screen.

Exception

  • User Action: ui error
  • Request: App crash, UI failure
  • Content Language: GB
  • Service: none
  • Version: 0.14.2
  • OS: Linux Android 5.0.1 - 21
Crash log

java.lang.NullPointerException: Attempt to invoke interface method 'android.view.MenuItem android.view.MenuItem.setVisible(boolean)' on a null object reference
	at us.shandian.giga.ui.adapter.MissionAdapter.onBindViewHolder(MissionAdapter.java:145)
	at android.support.v7.widget.RecyclerView$Adapter.onBindViewHolder(RecyclerView.java:6781)
	at android.support.v7.widget.RecyclerView$Adapter.bindViewHolder(RecyclerView.java:6823)
	at android.support.v7.widget.RecyclerView$Recycler.tryBindViewHolderByDeadline(RecyclerView.java:5752)
	at android.support.v7.widget.RecyclerView$Recycler.tryGetViewHolderForPositionByDeadline(RecyclerView.java:6019)
	at android.support.v7.widget.RecyclerView$Recycler.getViewForPosition(RecyclerView.java:5858)
	at android.support.v7.widget.RecyclerView$Recycler.getViewForPosition(RecyclerView.java:5854)
	at android.support.v7.widget.LinearLayoutManager$LayoutState.next(LinearLayoutManager.java:2230)
	at android.support.v7.widget.GridLayoutManager.layoutChunk(GridLayoutManager.java:557)
	at android.support.v7.widget.LinearLayoutManager.fill(LinearLayoutManager.java:1517)
	at android.support.v7.widget.LinearLayoutManager.onLayoutChildren(LinearLayoutManager.java:612)
	at android.support.v7.widget.GridLayoutManager.onLayoutChildren(GridLayoutManager.java:171)
	at android.support.v7.widget.RecyclerView.dispatchLayoutStep2(RecyclerView.java:3924)
	at android.support.v7.widget.RecyclerView.dispatchLayout(RecyclerView.java:3641)
	at android.support.v7.widget.RecyclerView.onLayout(RecyclerView.java:4194)
	at android.view.View.layout(View.java:15905)
	at android.view.ViewGroup.layout(ViewGroup.java:5108)
	at android.widget.LinearLayout.setChildFrame(LinearLayout.java:1959)
	at android.widget.LinearLayout.layoutVertical(LinearLayout.java:1813)
	at android.widget.LinearLayout.onLayout(LinearLayout.java:1722)
	at android.view.View.layout(View.java:15905)
	at android.view.ViewGroup.layout(ViewGroup.java:5108)
	at android.widget.FrameLayout.layoutChildren(FrameLayout.java:633)
	at android.widget.FrameLayout.onLayout(FrameLayout.java:568)
	at android.view.View.layout(View.java:15905)
	at android.view.ViewGroup.layout(ViewGroup.java:5108)
	at android.widget.LinearLayout.setChildFrame(LinearLayout.java:1959)
	at android.widget.LinearLayout.layoutVertical(LinearLayout.java:1813)
	at android.widget.LinearLayout.onLayout(LinearLayout.java:1722)
	at android.view.View.layout(View.java:15905)
	at android.view.ViewGroup.layout(ViewGroup.java:5108)
	at android.widget.FrameLayout.layoutChildren(FrameLayout.java:633)
	at android.widget.FrameLayout.onLayout(FrameLayout.java:568)
	at android.view.View.layout(View.java:15905)
	at android.view.ViewGroup.layout(ViewGroup.java:5108)
	at android.widget.LinearLayout.setChildFrame(LinearLayout.java:1959)
	at android.widget.LinearLayout.layoutVertical(LinearLayout.java:1813)
	at android.widget.LinearLayout.onLayout(LinearLayout.java:1722)
	at android.view.View.layout(View.java:15905)
	at android.view.ViewGroup.layout(ViewGroup.java:5108)
	at android.widget.FrameLayout.layoutChildren(FrameLayout.java:633)
	at android.widget.FrameLayout.onLayout(FrameLayout.java:568)
	at android.view.View.layout(View.java:15905)
	at android.view.ViewGroup.layout(ViewGroup.java:5108)
	at android.widget.LinearLayout.setChildFrame(LinearLayout.java:1959)
	at android.widget.LinearLayout.layoutVertical(LinearLayout.java:1813)
	at android.widget.LinearLayout.onLayout(LinearLayout.java:1722)
	at android.view.View.layout(View.java:15905)
	at android.view.ViewGroup.layout(ViewGroup.java:5108)
	at android.widget.FrameLayout.layoutChildren(FrameLayout.java:633)
	at android.widget.FrameLayout.onLayout(FrameLayout.java:568)
	at android.view.View.layout(View.java:15905)
	at android.view.ViewGroup.layout(ViewGroup.java:5108)
	at android.view.ViewRootImpl.performLayout(ViewRootImpl.java:2425)
	at android.view.ViewRootImpl.performTraversals(ViewRootImpl.java:2131)
	at android.view.ViewRootImpl.doTraversal(ViewRootImpl.java:1256)
	at android.view.ViewRootImpl$TraversalRunnable.run(ViewRootImpl.java:6443)
	at android.view.Choreographer$CallbackRecord.run(Choreographer.java:800)
	at android.view.Choreographer.doCallbacks(Choreographer.java:603)
	at android.view.Choreographer.doFrame(Choreographer.java:572)
	at android.view.Choreographer$FrameDisplayEventReceiver.run(Choreographer.java:786)
	at android.os.Handler.handleCallback(Handler.java:815)
	at android.os.Handler.dispatchMessage(Handler.java:104)
	at android.os.Looper.loop(Looper.java:194)
	at android.app.ActivityThread.main(ActivityThread.java:5576)
	at java.lang.reflect.Method.invoke(Native Method)
	at java.lang.reflect.Method.invoke(Method.java:372)
	at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:956)
	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:751)


PS: please note my edit in the last comment :)

@kapodamy

This comment has been minimized.

Copy link
Contributor Author

kapodamy commented Nov 20, 2018

@TobiGr

  • implemented
  • implemented
  • re-implemented, but works only if the required streams sizes are fetched (already displayed in the download dialog). In weak networks maybe no.
  • about Show info, this also happens when navigating between suggested videos. can't fix this
@kapodamy

This comment has been minimized.

Copy link
Contributor Author

kapodamy commented Nov 20, 2018

@TobiGr about the errors
1.It seems that NewPipe dies when the screen goes off. Once the postprocessing is started, it can not be stopped or the file will be damaged (error 1007). edit: needs a wakelock implementation
2. looks like a race-condition, should be fixed now

@TobiGr TobiGr referenced this pull request Nov 22, 2018

Closed

Are/will CC be available? #1833

0 of 1 task complete
more fixes
* use bold style in status (mission_item_linear.xml)
* fix download attemps not begin updated
* dont stop the queue if a download fails
* implement partial wake-lock & wifi-lock
* show notifications for failed downloads
* ¿proper bitmap dispose? (DownloadManagerService.java)
* improve buffer filling (CircularFile.java)
* [Mp4Dash] increment reserved space from 2MiB to 15MiB. This is expensive but useful for devices with low ram
* [WebM] use 2MiB of reserved space
* fix debug warning if one thread is used
* fix wrong download speed when the activity is suspended
* Fix "Queue" menu item that appears in post-processing errors
* fix mission length dont being updated (missing commit)
@TobiGr

This comment has been minimized.

Copy link
Member

TobiGr commented Nov 23, 2018

@kapodamy It good to see that you were able to fix the crashes. Unfortunately, I produced some more :)
Just to note, I reviewed this at the state of 767dc20 (Thursday morning). So in case you fixed any of the following, don't be mad with me :)
Let's start with small things like UX and bugs at last:

  • In the settings you added the option to set the maximum number of retries when a download failed. When changing the number via the seek bar, the number on the right is not updated simultaneously.

download_settings_retries

  • There are some more issues with the light theme which I didn't recognize before.

download_app_bar_theme

  • There's also another small theme bug, but I guess we can open a separate ticket for it. After opening the downloads view, go to the settings via the 3-dot-menu in the action bar. Change a theme and go back to downloads via the back buttons. You'll see that the new theme was not applied. I didn't check it, but I guess this issue existed in previous versions, too.

  • You can get the following exception when taping on a tile of a finished download. This crash does not appear when using the tile menu and then pressing play. I am not sure if this crash was there before. Nevertheless we are not able to "play" subtitles. So neither tapping on a subtitle tile or using the play option via the menu should be possible.

Exception

  • User Action: ui error
  • Request: App crash, UI failure
  • Content Language: GB
  • Service: none
  • Version: 0.14.2
  • OS: Linux Android 5.0.1 - 21
Crash log

java.lang.ClassCastException: us.shandian.giga.get.FinishedMission cannot be cast to us.shandian.giga.get.DownloadMission
	at us.shandian.giga.ui.adapter.MissionAdapter$ViewHolderItem.lambda$new$1(MissionAdapter.java:591)
	at us.shandian.giga.ui.adapter.-$$Lambda$MissionAdapter$ViewHolderItem$FHK8sq2xsmJ5bNl5JAWgRX95u8I.onClick(lambda)
	at android.view.View.performClick(View.java:4807)
	at android.view.View$PerformClick.run(View.java:20106)
	at android.os.Handler.handleCallback(Handler.java:815)
	at android.os.Handler.dispatchMessage(Handler.java:104)
	at android.os.Looper.loop(Looper.java:194)
	at android.app.ActivityThread.main(ActivityThread.java:5576)
	at java.lang.reflect.Method.invoke(Native Method)
	at java.lang.reflect.Method.invoke(Method.java:372)
	at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:956)
	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:751)


Download bugs

There are issues with large downloads on Android 5. I experienced strange behaviours with streams >= 3 GB. As you can see, the download state is not displayed correctly and the current progress is not displayed either. Additionally, the download does not finish and at some point the download size (on the bottom left) grows every second. When opening the downloads view, for half a second or so, the current file site is displayed at the position of the current download speed in GB/s. I guess all these bugs are related to the incorrect state. I tested this with Android 5 and bad WiFi. Both are not optimal testing conditions, so in case you cannot reproduce this, I can try to create some more logs for you.

download_wrong_state
download_wrong_stream size


I'll take a look at your latest changes, which I didn't test, next week.

@kapodamy

This comment has been minimized.

Copy link
Contributor Author

kapodamy commented Nov 24, 2018

@TobiGr

  1. workarrond using a list.
  2. cant fix
  3. the line MissionAdapter.java:591 is oudated for me, maybe fixed now (invalid casting)
  4. float overflow, double will be used. Very expensive
and more fixes
* fix content length reading
* use float overflow. Expensive, double is used instead
* fix invalid cast after click the mission body
* use a list for maximum attemps (downloads)
* minor clean up (DownloadManager.java)
* dont pass SharedPreferences instace to DownloadManager
* use a switch instead of checkbox for cross_network_downloads
* notify media scanner after deleting a finished download

kapodamy added some commits Dec 5, 2018

fast download pausing
* fast download pausing
* fix UI thread blocking when calling pause()
* check running threads before start the download
* fix null pointer exception in onDestroy in the download service, without calling onCreate method (android 8)
@kapodamy

This comment has been minimized.

Copy link
Contributor Author

kapodamy commented Dec 5, 2018

@theScrabi
when you have time, please take a look

@theScrabi

This comment has been minimized.

Copy link
Member

theScrabi commented Dec 11, 2018

giphy
done.
Great work thx 🎉

@theScrabi theScrabi merged commit 3599ab3 into TeamNewPipe:dev Dec 11, 2018

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
security/snyk - app/build.gradle (theScrabi) No known issues
Details
security/snyk - build.gradle (theScrabi) No known issues
Details
@theScrabi

This comment has been minimized.

Copy link
Member

theScrabi commented Dec 11, 2018

The only issue is that the icons in the download manager do not adapt bright design. Pleas fix this in an additional commit.

screenshot_20181211-163029

@FaizKhan5

This comment has been minimized.

Copy link

FaizKhan5 commented Dec 15, 2018

is it able to download audio in mp3 ?

@kapodamy

This comment has been minimized.

Copy link
Contributor Author

kapodamy commented Dec 15, 2018

@FaizKhan5 if you mean convert m4a to mp3 no.

@FaizKhan5

This comment has been minimized.

Copy link

FaizKhan5 commented Dec 15, 2018

@kapodamy yes, i mean that. Is there any chance to implement that in near future ?

@pro4tlzz

This comment has been minimized.

Copy link

pro4tlzz commented Feb 12, 2019

Thanks for the update

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.