-
Notifications
You must be signed in to change notification settings - Fork 417
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
Support delivery methods other than progressive HTTP #810
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reviewed the first batch. Note that I did not check for spelling or grammar.
extractor/src/main/java/org/schabi/newpipe/extractor/stream/DeliveryMethod.java
Show resolved
Hide resolved
extractor/src/main/java/org/schabi/newpipe/extractor/services/youtube/DashMpdParser.java
Outdated
Show resolved
Hide resolved
extractor/src/main/java/org/schabi/newpipe/extractor/stream/AudioStream.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that I did not check for spelling or grammar.
Don't worry, I've got your back, @TobiGr. ;)
.../org/schabi/newpipe/extractor/services/media_ccc/extractors/MediaCCCLiveStreamExtractor.java
Outdated
Show resolved
Hide resolved
.../org/schabi/newpipe/extractor/services/media_ccc/extractors/MediaCCCLiveStreamExtractor.java
Outdated
Show resolved
Hide resolved
.../org/schabi/newpipe/extractor/services/media_ccc/extractors/MediaCCCLiveStreamExtractor.java
Outdated
Show resolved
Hide resolved
.../org/schabi/newpipe/extractor/services/media_ccc/extractors/MediaCCCLiveStreamExtractor.java
Outdated
Show resolved
Hide resolved
...java/org/schabi/newpipe/extractor/services/media_ccc/extractors/MediaCCCStreamExtractor.java
Outdated
Show resolved
Hide resolved
extractor/src/main/java/org/schabi/newpipe/extractor/stream/SubtitlesStream.java
Outdated
Show resolved
Hide resolved
extractor/src/main/java/org/schabi/newpipe/extractor/stream/SubtitlesStream.java
Outdated
Show resolved
Hide resolved
extractor/src/main/java/org/schabi/newpipe/extractor/stream/SubtitlesStream.java
Outdated
Show resolved
Hide resolved
extractor/src/main/java/org/schabi/newpipe/extractor/stream/SubtitlesStream.java
Outdated
Show resolved
Hide resolved
extractor/src/main/java/org/schabi/newpipe/extractor/stream/SubtitlesStream.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well done!
extractor/src/main/java/org/schabi/newpipe/extractor/services/youtube/DashMpdParser.java
Outdated
Show resolved
Hide resolved
...in/java/org/schabi/newpipe/extractor/services/youtube/extractors/YoutubeStreamExtractor.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some more.
.../java/org/schabi/newpipe/extractor/services/peertube/extractors/PeertubeStreamExtractor.java
Outdated
Show resolved
Hide resolved
.../java/org/schabi/newpipe/extractor/services/peertube/extractors/PeertubeStreamExtractor.java
Outdated
Show resolved
Hide resolved
.../java/org/schabi/newpipe/extractor/services/peertube/extractors/PeertubeStreamExtractor.java
Outdated
Show resolved
Hide resolved
.../java/org/schabi/newpipe/extractor/services/peertube/extractors/PeertubeStreamExtractor.java
Outdated
Show resolved
Hide resolved
extractor/src/main/java/org/schabi/newpipe/extractor/services/youtube/ItagItem.java
Outdated
Show resolved
Hide resolved
extractor/src/main/java/org/schabi/newpipe/extractor/services/youtube/ItagItem.java
Show resolved
Hide resolved
extractor/src/main/java/org/schabi/newpipe/extractor/services/youtube/ItagItem.java
Outdated
Show resolved
Hide resolved
extractor/src/main/java/org/schabi/newpipe/extractor/services/youtube/ItagItem.java
Outdated
Show resolved
Hide resolved
extractor/src/main/java/org/schabi/newpipe/extractor/services/youtube/ItagItem.java
Outdated
Show resolved
Hide resolved
extractor/src/main/java/org/schabi/newpipe/extractor/services/youtube/ItagItem.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the PR.
A lot better than the old one but still some problems (and too big IMHO).
What is the advantage of this exactly?
Because currently I see none - despite having 6k new lines of code to maintain...
I wouldn't merge this PR without having Sonarcloud generate a report for it.
.../java/org/schabi/newpipe/extractor/services/bandcamp/extractors/BandcampStreamExtractor.java
Outdated
Show resolved
Hide resolved
.../org/schabi/newpipe/extractor/services/media_ccc/extractors/MediaCCCLiveStreamExtractor.java
Outdated
Show resolved
Hide resolved
.../org/schabi/newpipe/extractor/services/media_ccc/extractors/MediaCCCLiveStreamExtractor.java
Outdated
Show resolved
Hide resolved
.../org/schabi/newpipe/extractor/services/media_ccc/extractors/MediaCCCLiveStreamExtractor.java
Outdated
Show resolved
Hide resolved
.../java/org/schabi/newpipe/extractor/services/peertube/extractors/PeertubeStreamExtractor.java
Outdated
Show resolved
Hide resolved
extractor/src/main/java/org/schabi/newpipe/extractor/stream/Stream.java
Outdated
Show resolved
Hide resolved
extractor/src/main/java/org/schabi/newpipe/extractor/utils/ManifestCreatorCache.java
Outdated
Show resolved
Hide resolved
.../test/java/org/schabi/newpipe/extractor/services/youtube/YoutubeDashManifestCreatorTest.java
Outdated
Show resolved
Hide resolved
.../test/java/org/schabi/newpipe/extractor/services/youtube/YoutubeDashManifestCreatorTest.java
Outdated
Show resolved
Hide resolved
extractor/src/test/java/org/schabi/newpipe/extractor/utils/UtilsTest.java
Outdated
Show resolved
Hide resolved
It fixes Peertube seeking. Currently, if you try to seek in a video, it just takes you to the tip of the buffer, like a livestream. You can only seek properly in the DASH APK. Edit: Oh, and it also restores streams on certain music (and other) videos on YT. OTF streams were disabled on an earlier 0.20.x build, so currently, some videos only have 720p MP4, 360p MP4, and 144p 3GPP streams. The DASH APK shows and plays all MP4 and WebM streams. |
@litetex those are needed to distinguish between different streams in NewPipe, see TeamNewPipe/NewPipe#6537 (comment) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Partial review, will finish later/tomorrow/(the day after)
.../org/schabi/newpipe/extractor/services/media_ccc/extractors/MediaCCCLiveStreamExtractor.java
Outdated
Show resolved
Hide resolved
extractor/src/main/java/org/schabi/newpipe/extractor/services/youtube/YoutubeParsingHelper.java
Outdated
Show resolved
Hide resolved
.../test/java/org/schabi/newpipe/extractor/services/youtube/YoutubeDashManifestCreatorTest.java
Outdated
Show resolved
Hide resolved
.../test/java/org/schabi/newpipe/extractor/services/youtube/YoutubeDashManifestCreatorTest.java
Outdated
Show resolved
Hide resolved
@Stypox
The argument also doesn't work for me because there is also currently no overridden I'm so nitpicking about these because I had similar cases (in other software) in the past: E.g. someone created an object slightly different and the persistence framework failed to save these objects correctly because |
@litetex What? There are overridden in all Note that I added more checks in the methods locally in the @Override
public boolean equals(final Object obj) {
if (this == obj) {
return true;
}
if (obj == null || getClass() != obj.getClass()) {
return false;
}
final Stream stream = (Stream) obj;
return id.equals(stream.id) && mediaFormat == stream.mediaFormat
&& deliveryMethod == stream.deliveryMethod
&& content.equals(stream.content)
&& isUrl == stream.isUrl
&& Objects.equals(baseUrl, stream.baseUrl);
}
@Override
public int hashCode() {
return Objects.hash(id, mediaFormat, deliveryMethod, content, isUrl, baseUrl);
} |
No. |
@litetex You're looking on the dev branch, I am implementing these methods in the PR 😅 |
My knowledge in unit tests is like nothing, so I am not able to write good tests, I'm afraid for this. Due to my lack of time, I am not really able to maintain this PR for now (I will push most of the requested changes and rebase the PR however, so please wait before doing something until it's done). So someone else will have to apply the final changes in this PR and the app part of the changes. Note that when #780 is merged, you will have to add, in |
5e3740b
to
5dfa3bc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reviewed everything, though I can't really confirm that the service-specific code work well (which this PR is mostly about). Tests should come in handy for that purpose, so I would be ok with merging this even if I didn't try every possible service request myself but just let the tests do their job.
The code structure looks good, though maybe sometimes the javadocs are not much to-the-point, and there are many methods being added and never used (such as all of the different ways to clear cache, change clear factor, ... in YoutubeDashManifestCreator
). But these two problems are arguably just my opinion and not really big problems anyway.
.../org/schabi/newpipe/extractor/services/media_ccc/extractors/MediaCCCLiveStreamExtractor.java
Outdated
Show resolved
Hide resolved
extractor/src/main/java/org/schabi/newpipe/extractor/services/youtube/DashMpdParser.java
Outdated
Show resolved
Hide resolved
extractor/src/main/java/org/schabi/newpipe/extractor/services/youtube/ItagItem.java
Show resolved
Hide resolved
.../src/main/java/org/schabi/newpipe/extractor/services/youtube/YoutubeDashManifestCreator.java
Outdated
Show resolved
Hide resolved
extractor/src/main/java/org/schabi/newpipe/extractor/services/youtube/YoutubeParsingHelper.java
Outdated
Show resolved
Hide resolved
extractor/src/main/java/org/schabi/newpipe/extractor/stream/DeliveryMethod.java
Outdated
Show resolved
Hide resolved
extractor/src/main/java/org/schabi/newpipe/extractor/utils/ManifestCreatorCache.java
Outdated
Show resolved
Hide resolved
extractor/src/test/java/org/schabi/newpipe/extractor/utils/UtilsTest.java
Outdated
Show resolved
Hide resolved
.../test/java/org/schabi/newpipe/extractor/services/youtube/YoutubeDashManifestCreatorTest.java
Outdated
Show resolved
Hide resolved
I'm not talking about that these methods are overridden in a single class and not in another. Read #810 (comment) |
5dfa3bc
to
1fb74eb
Compare
79151b5
to
13332b4
Compare
…e in ItagItems generated This change allows to build DASH manifests using YoutubeDashManifestCreator with the real duration of streams and prevent potential cuts of the end of progressive streams, because the duration in YouTube's player response is in seconds and not milliseconds.
Move DASH manifests creation into a new subpackage of the YouTube package, dashmanifestcreators. This subpackage contains: - CreationException, exception extending Java's RuntimeException, thrown by manifest creators when something goes wrong; - YoutubeDashManifestCreatorsUtils, class which contains all common methods and constants of all or a part of the manifest creators; - a manifest creator has been added per delivery type of YouTube streams: - YoutubeProgressiveDashManifestCreator, for progressive streams; - YoutubeOtfDashManifestCreator, for OTF streams; - YoutubePostLiveStreamDvrDashManifestCreator, for post-live DVR streams (which use the live delivery method). Every DASH manifest creator has a getCache() static method, which returns the ManifestCreatorCache instance used to cache results. DeliveryType has been also extracted from the YouTube DASH manifest creators part of the extractor and moved to the YouTube package. YoutubeDashManifestCreatorTest has been updated and renamed to YoutubeDashManifestCreatorsTest, and YoutubeDashManifestCreator has been removed. Finally, several documentation and exception messages fixes and improvements have been made.
…be DASH manifest creators
…only a single stream URL from HLS manifest for MP3 streams SoundCloud broke the workaround used to get a single file from HLS manifests for Opus manifests, but it still works for MP3 ones. The code has been adapted to prevent an unneeded request (the one to the Opus HLS manifest) and the HLS delivery method is now used for SoundCloud MP3 and Opus streams, plus the progressive one (for tracks which have a progressive stream (MP3) and for the ones which doesn't have one, it is still used by trying to get a progressive stream, using the workaround). Streams extraction has been also moved to Java 8 Stream's API and the relevant test has been also updated.
2cbb6ba
to
287d1df
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@litetex please merge unless you can find other critical problems. Even if you can still possibly find some minor things to improve, I would merge this anyway and you can directly create a small PR improving such small things (which takes less effort than making a review).
Damn, DASH support has finally been added after almost 2 years (Mauricio's PR was on 21 June). Mad props to everyone involved for all the effort! |
These fields were added in TeamNewPipe#810 in preparation for a DASH support that never materialized … well it did materialize, but did not use any of these variables. Since they are youtube-specific, it does not make much sense to export them in the generic API, lest people start depending on them as if they belonged to all backends. Well, 4 variables are actually already depended on in places, they will have to be checked separately. But this is the low-hanging fruit.
These fields were added in TeamNewPipe#810 in preparation for a DASH support that never materialized … well it did materialize, but did not use any of these variables. Since they are youtube-specific, it does not make much sense to export them in the generic API, lest people start depending on them as if they belonged to all backends. Well, 4 variables are actually already depended on in places, they will have to be checked separately. But this is the low-hanging fruit.
Previous too-long title: Add support of other delivery methods than progressive HTTP (DASH, HLS and SmoothStreaming), support non-URLs contents and more
This PR superseeds #663 with more and less changes and improvements.
@wb9688 has been the initiator of this feature/enhancement, thank you again for your work!
This PR mainly adds support of other delivery methods than progressive HTTP (DASH, HLS and SmoothStreaming) and of non-URLs content in the extractor but also:
(except SoundCloud, at least for now);ItagItem
, to generate better and more types of DASH manifests using the extractor;Stream
objects (classes which are extended theStream
class) are built (⚠ it's a breaking change for clients which buildStream
objects!Stream
classes (replaced by getters and/or setters);getUrl()
method of theStream
class (use thegetContent()
method instead and theisUrl()
one to check if the content is a URL or not), which returns nownull
if the stream's content is not a URL;Summary of (important) code changes (check out the code changes for more details):
New (sub)packages:
dashmanifestcreators
(in theyoutube
service package), which contains the YouTube DASH manifest creators of YouTube streams (see below).New enums:
DeliveryMethod
(in thestream
package), to represent the delivery method of contents extracted. You can get it in theStream
class, by using thegetDeliveryMethod()
method;DeliveryType
(in theyoutube
service package), which contains the stream delivery types of YouTube streams (which are used by manifest creators). This enum is not representing the delivery method of streams, which is the job of the enum above;New classes:
ItagInfo
, (in theyoutube
service package) a class which contains everything needed to build YouTube streams (intended to be only used internally): the content, whether the content is a URL and anItagItem
;ManifestCreatorCache
(in theutils
package), a class to cache results of a manifest creator (types of keys and values must be serializable), which allow setting a cache limit, a clear factor and more;Pair
(in theutils
package), a class to store and access a pair of serializable objects (this class is intended to be only used internally);YoutubeProgressiveDashManifestCreator
,YoutubeOtfDashManifestCreator
andYoutubePostLiveStreamDvrDashManifestCreator
(in thedashmanifestcreators
YouTube service package), classes to generate respectively DASH manifests of YouTube progressive, OTF, and post-live streams. Common methods are present in a separate class,YoutubeDashManifestCreatorsUtils
(in thedashmanifestcreators
YouTube service package too). YouTube DASH manifest creators cache results and allow setting limits to the cache (by using the methods ofManifestCreatorCache
instance used);CreationException
(in thedashmanifestcreators
YouTube service package), aRuntimeException
which is thrown when an exception occured in YouTube DASH manifest creators;YoutubeDashManifestCreatorsTest
(in theyoutube
service test package), to test what we can test of testable YouTube DASH manifest creators (see the code of the test class for more information).Changes in the
StreamType
enum:POST_LIVE_STREAM
andPOST_LIVE_AUDIO_STREAM
values to the end of the enum and document this enum.FILE
value.Changes in the
ItagItem
class:AVERAGE_BITRATE_UNKNOWN
,SAMPLE_RATE_UNKNOWN
,FPS_NOT_APPLICABLE_OR_UNKNOWN
,TARGET_DURATION_SEC_UNKNOWN
,APPROX_DURATION_MS_UNKNOWN
,AUDIO_CHANNELS_NOT_APPLICABLE_OR_UNKNOWN
andCONTENT_LENGTH_UNKNOWN
;getAverageBitrate()
,getFps()
,setFps(int)
,getResolutionString()
,getSampleRate()
,setSampleRate(int)
,getAudioChannels()
,setAudioChannels(int)
,getTargetDurationSec()
,setTargetDurationSec(int)
,getApproxDurationMs()
,setApproxDurationMs(long)
,getContentLength()
andsetContentLength(long)
;avgBitrate
,fps
andresolutionString
. UsegetAverageBitrate()
,getFps()
,setFps(int)
andgetResolutionString()
instead.Changes in the
Stream
classes:In the
Stream
abstract class:FORMAT_ID_UNKNOWN
,ID_UNKNOWN
andITAG_NOT_AVAILABLE_OR_NOT_APPLICABLE
;getId()
,getContent()
,isUrl()
,getDeliveryMethod()
,getBaseUrl()
andgetItagItem()
(abstract method which is implemented in the classes which extend this class). Check out the corresponding documentations to know and understand what they return;getUrl()
, which may return nownull
. The extractor is able to extract non-URL contents, so if you call this method on a non-URL content, it will returnnull
. UsegetContent()
instead andisUrl()
to know whether the content is the URL to the content or the content itself.equals(Stream)
. UseequalStats(Stream)
to compare statistics of two streams and the Java standardequals(Object)
method to compare the equality of two streams instead.In classes which extends the
Stream
class:Stream
class!Add new classes to build objects of these classes. See the documentations of each class and their methods to know what you need to set and what you can set to build these
Builder
s;Remove the ability to build objects of theses classes with constructors. Use the
Builder
classes instead, which may also prevent you to set values that are default.In the
AudioStream
class:UNKNOWN_BITRATE
.In the
VideoStream
class:RESOLUTION_UNKNOWN
;isVideoOnly
andresolution
fields. Use the corresponding getters instead,isVideoOnly()
andgetResolution()
.Changes in
StreamExtractor
s:StreamExtractor
classes and improve some of their code (check out the code changes by yourself for more details).Changes in
StreamExtractor
test classes:DefaultStreamExtractorTest
andSoundcloudStreamExtractorTest
to consider the changes made inStream
classes (check out the code changes for more details).Changes in other test classes:
ManifestCreatorCache
class inUtilsTest
(check out the code changes for more details);DashMpdParser
, in which the parsing of YouTube DASH manifest was initially fixed (and is still available in Git history).Closes #223, closes #273, closes #461, fixes #648.