-
-
Notifications
You must be signed in to change notification settings - Fork 496
[SoundCloud] Fix SoundCloud HLS expiry and add extractor logging #1325
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
base: dev
Are you sure you want to change the base?
[SoundCloud] Fix SoundCloud HLS expiry and add extractor logging #1325
Conversation
Add StreamingServiceId enum to map service id to name for logging
…rate building Hls and Progressive streams Add HlsAudioStream to facilitate refreshing expired Hls playlists
45df3cd to
578f4f0
Compare
| logger.error(tag, msg, t); | ||
| } | ||
|
|
||
| // default logger that prints to stdout |
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.
| // default logger that prints to stdout | |
| /** | |
| * Default logger that prints to stdout. | |
| */ |
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! My review gives some thoughts that we should decide on before continuing. There definitely needs to be a way to refresh streams in the extractor, which will come in handy not only for SoundCloud but also for YouTube.
I think it is a good idea to store refresh information in the streams themselves, and to make them expose methods that allow refreshing the stream. I guess this would also play well with #858 (comment). What do you think @AudricV?
Regarding logging, if I understand correctly the commit structure, it should be possible to extract it in a separate PR, that we can merge much faster. Could you put the logging in a separate PR? Also the
| public static void d(final String tag, final String msg) { | ||
| logger.debug(tag, msg); | ||
| } | ||
|
|
||
| public static void w(final String tag, final String msg) { | ||
| logger.warn(tag, msg); | ||
| } |
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.
These should also optionally take a Throwable (that's how they work in Android, and it's often useful)
| @SuppressWarnings("checkstyle:LeftCurly") | ||
| public interface RefreshableStream { |
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.
| @SuppressWarnings("checkstyle:LeftCurly") | |
| public interface RefreshableStream { | |
| public interface RefreshableStream { |
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.
Is this file used anywhere? Also, the reason why services are not implemented as an enum is also because it should be possible to dynamically add service implementations (think e.g. of plugins). Obviously this has never happened yet, but iirc that's why the API is like that.
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.
It's used in the NewPipe PR for logging the names of services.
It's possible to have enums and still allow for dynamic implementations. Since plugins are a long way away, I think it makes a lot more sense to have enums for each service to improve readability, traceability, logging and type safety.
Howbeit, I'm not going to change the entire API to use enums because that's long: I just want the nice id -> name static method.
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.
Can't you use this utility method instead? https://github.com/TeamNewPipe/NewPipe/blob/2a9c6f05387866bef2c42ab67226c14936917d27/app/src/main/java/org/schabi/newpipe/util/ServiceHelper.java#L138
But yeah I agree this is not the best. We should not have the concept of service ID at all except for saving stuff to database, so every StreamInfo should not have any getServiceId() method but rather getService() directly. So we don't have to keep switching back and forth between int and Service
| } | ||
|
|
||
| // default logger that prints to stdout | ||
| private static final class ConsoleLogger implements Logger { |
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.
This should be only set by users of the library and should default to a logger that logs nothing. In each extractor test then you would set this ConsoleLogger as the logger (i.e. you just add this in the common tests initialization methods).
|
|
||
| import javax.annotation.Nonnull; | ||
|
|
||
| public class HlsAudioStream extends AudioStream implements RefreshableStream { |
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 don't think it's a property of all HLS streams that they are refreshable. I think we have two alternative ways we can handle this instead:
RefreshableStreamcould just be an interface that someStreams can implement, then eachServicewould overrideVideoStream/AudioStreamand also implementRefreshableStreamif needed, and the player could check for refreshable streams usinginstanceof. This would require a lot of boilerplate though.- Add a
(Audio/Video)Stream refresh() {}method to the baseStreamclass which builds a new refreshed stream, and make it returnnullif refreshing is not available.
| import java.io.IOException; | ||
|
|
||
| @SuppressWarnings("checkstyle:LeftCurly") | ||
| public interface RefreshableStream { |
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.
Shouldn't this also somehow store the expiry time? So that the player can prepare in advance if the stream is about to expire
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.
How exactly would the player prepare in advance? What is it supposed to do?
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.
If the currently playing stream expires at 18:50:45 and now it's 18:50:35, there are just 10 more seconds left to fetch data before needing to switch over to the refreshed stream. So instead of waiting until the URL returns 403 and then quitting, the player can notice that only 10 seconds are left and already pre-fetch the refreshed stream and switch to it as soon as it's ready.
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.
ExoPlayer handles buffering of stream data, so I don't think is necessary since stream chunks are already pre-fetched ahead of time.
It fetches chunks ahead of time, so when it gets a 403 it's not for the currently playing chunk.
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.
Oh right, that's true. I'd still add a nullable expiration Instant just in case other clients need it (or maybe NewPipe's Downloader might need it? Or maybe streams in other services that don't throw 403 and where it's more difficult to detect that the stream is now invalid?), since this is information the extractor usually knows. But this is not totally necessary, so do it only if it seems simple to extract such data.
| * | ||
| * @return the resolved id | ||
| */ | ||
| // TODO: what makes this method different from the others? Don' they all return the same? |
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.
Maybe you can get an answer by git blaming?
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've found answer to this question as per #1322 (comment)
Seems you didn't finish your sentence there lol. Yeah sure I can make a separate logging PR that can be merged and then rebase this PR on top of it |
|
I have no idea what I wanted to say now, sorry xD |
Fix SoundCloud HLS stream urls expiring
This PR implements extractor side changes to facilitate refreshing expires HLS stream URLs for SoundCloud streams.
Add Extractor Logging
Added extractor logging, which helped a lot to fix this issue.
In a future PR I will add a proper logging framework that will only print logs if running in debug mode, but for now since the logs are not in high-traffic code then it should be fine.
Please Note
Includes changes from:
So those PRs must be merged before this.
See TeamNewPipe/NewPipe#12418 for the full writeup for the fix