-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
plugins.chzzk: new plugin #5731
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.
Thanks for the PR, but a plugin for this site can't be merged yet:
https://github.com/streamlink/streamlink/blob/6.5.0/CONTRIBUTING.md#plugin-requests
9. Sites which are unmaintained, are in beta or are undergoing heavy amounts of development and may change rapidly
I'm not going to review this yet, but I can already tell you that there are several code-linting issues which need to get fixed.
The validation schema also requires a couple of updates, assuming that the site's API stays that way. There's no error handling of invalid user inputs (unknown/offline channels) and apparently there are different HLS playlist URLs for regular and low-latency streams.
@bastimeyer I've fixed code lint and some exception handling, and added VOD feature. |
6fe01cc
to
e0008e3
Compare
Thanks for the update. There are still quite a few issues left though. Instead of reviewing and annotating the code issues, I decided to update the plugin and put it into proper shape myself, so it gets done quicker. I therefore force-pushed the changes onto your PR branch (which is master). This however doesn't mean that the plugin will get merged into Streamlink's master branch now. The site is still in beta, and a working plugin doesn't change this fact. Btw, good catch with the MPEG2 transport stream adaptation sets of the VOD DASH streams. The low latency live HLS streams are currently not supported by Streamlink, because the new HLS spec hasn't been finalized yet and partial segments have not been implemented here yet. The LLHLS playlists could be used instead, as it's backwards compatible, but it wouldn't make a difference. Quick verification with a random stream and VOD:
|
Thank you for fixing the code to be more concise. FYI, Chzzk is expected to be officially released in February. Until then, I leave this PR in draft status for the reason that there might be any follow-ups, such as changes to the API specification. |
It seems that chzzk login authentication has been added. If logged in, information will be retrieved, but otherwise, information will not be retrieved. |
No, they simply added a
|
They changed more than just v1 to v2. With v2 they introduced 19+ streams (like AfreecaTV). To watch these streams, authentication is required. The name and birthday of the authenticated account has to be verified by Naver once: https://help.naver.com/service/5640/contents/20897?lang=en 19+ streams won't return the content.livePlaybackJson part in the json when https://api.chzzk.naver.com/service/v2/channels/{channel_id}/live-detail is called. It seems GET requests to https://api.chzzk.naver.com/service/v2/channels/{channel_id}/live-detail require a cookie to return content.livePlaybackJson for 19+ streams. In order to watch such streams, pass the parameter
|
After exactly 9 hours, the stream breaks. Log is below. It seems some kind of expiration date is kicking in (hdntl=exp=1706567743 in my case). Is there anything that can be done to renew this before it expires?
|
@umemoni256, |
@umemoni256 @hoonnn @bastimeyer Hey guys. The token value expired 9 hours after it was issued, causing the recording to cut off in the middle. I revamped the code to reissue the token 3 hours before it expired and replace it in the URL fetching on the HLS playlist. I would appreciate some testing and code review. |
Sorry, but I'm not going to merge these newly added changes here. Please restore the branch again to the previous commit or I will do it once this plugin is ready to be merged. The initial plugin implementation should be focused on the basics, which commit 150edc9 achieved just fine (unless something has changed since it was created). An expiration timeout after 9 hours is considered an edge-case. Streamlink was never built for recording streams and it's still not the focus of the project today. This is partly the reason why the stream implementations are still lacking some extensibility capabilities. Just as a quick remark, in order for Streamlink to properly support recording which then can be advertised properly, many things will need to change on the side of both the streams API and CLI implementation. This however doesn't mean that the plugin can't be improved with refresh logic for the expiration timeout in the future. The way this is done here though is not good. I am aware that you are trying to reload the multivariant playlist, which is of course a bit tricky, but this proposed implementation, ignoring all the other linting, typing and method/function-signature issues, is having lots of issues which makes it impossible to merge. You are overriding a ton of HLSStream logic here, which makes it an unmaintainable mess once the base classes need to get updated, apart from the fact that lots of other stuff from the overridden methods is missing. A proper implementation should make sure that the base classes are extensible with a clean API, so subclasses can apply their extended logic which modifies the HLS stream behavior. Also, please keep the coding style of the project in mind when submitting changes. We don't have strict code style linters/formatters configured, but this doesn't mean that we're going to merge anything that the Python interpreter will parse. Annotating all these style issues is way too tedious. Btw, these changes here don't pass |
I understand that the points you've raised. Additionally, I recognize the challenges with linting and formatting due to local environment setup issues. I appreciate your intention to review for a Proof-of-Concept without fully considering code quality. However, I would like to clarify one thing:
If so, I've observed that other plugins within Streamlink that override HLSStream also deal with their expiration logic in ways that might seem tricky [1] [2] [3]. Could you specify what distinguishes my approach from theirs and pinpoint where the issue lies? |
src/streamlink/plugins/chzzk.py
Outdated
], | ||
}, | ||
validate.get("media"), | ||
validate.transform(lambda media: [(m["mediaId"], m["protocol"], m["path"]) for m in media]), |
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 can I use validate.union_get to extract the values of properties from the array inside media
? I'm looking for a way to efficiently retrieve these values in a tuple.
@bastimeyer I resolved the issue by referring to this PR |
Can it works in streamlink 6.2.0? |
@HDSRK No, it is not supported. Only Streamlink version >=6.6.0 are supported. If you want to use it with a lower version, you can manually add the code directly from this commit file into the plugins directory. However, it will not be officially supported though. |
No, it's not problematic, because playlist expirations are not part of the HLS specification and depend very much on how the streaming platform has implemented them. Plugins therefore always need custom logic in order to solve this type of issue. There can't be a generic solution. What I was saying was (in the context of your previous changes) that overloading new plugins with tons of extra code is bad for edge cases like this. It's an edge case because it's an expiration timer of 9 hours in contrast to only a couple of minutes for example, which would make the plugin unusable. I asked you to add playlist expiration logic after the basic plugin implementation has been merged, so extending it, reviewing the code and getting feedback is easier. Your recent changes however already look much better, but I haven't checked it in detail yet. I can already see a couple of issues though which will require fixing. But as said, I don't want to review this here.
Validation schema docs here: The initial commit already turned the array's objects into tuples, so the result could be destructed/unpacked later on: |
Muxed stream outputs can't be represented as a single URL, hence this error message when trying to use And btw, the current state of the plugin with its refresh-expiration logic on top of the basic plugin implementation won't be merged, as I've said multiple times now. |
I misunderstood what you meant at first, Thanks for your understanding @bastimeyer. I'll add the refresh-expiration logic after this PR is got to merge. |
This seemed to work fine until recently and now the error comes up even with the cookie values. Anyone experiencing same issue?
|
It seems like Chzzk is out of beta now. Is the plugin ready for merging? |
This PR will require a few modifications before it can get merged. I will have a look later today when I get back home and then update it myself. Is there an official statement somewhere that the site went out of its beta phase? |
Thanks! Here is also an easier to translate post: https://game.naver.com/lounge/chzzk/board/detail/4033764 |
Co-Authored-By: bastimeyer <mail@bastimeyer.de>
I've force-pushed onto your PR-branch, @fml09, which btw is The plugin should be fine now unless I've missed something. If you want to check and validate other streams, please go ahead. Then we can merge. Thanks.
|
Add plugin for chzzk which is the streaming platform operating in South Korea.
Resolves #5730