-
Notifications
You must be signed in to change notification settings - Fork 242
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
[Matrix] License Renewal for Youtube movies #589
Conversation
Ensure that Timer stops before CDM Adapter stops
@MisterD81 Thanks for the PR, sorry just getting to it now. Looks fine to me and doesn't crash but @phunkyfish could you have a once over whenever suits? I'm no expert in c++, just want to check the thread sleeping has no issue with you. MisterD81 do you know does the CDM call |
It is actually even a little bit more complicated. Initially you get keys that are valid for 5 minutes. |
@@ -63,10 +64,18 @@ void* GetCdmHost(int host_interface_version, void* user_data) | |||
|
|||
} // namespace | |||
|
|||
std::atomic<bool> exit_thread_flag{false}; |
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.
For simpler types regular assignment is easier to understand.
std::atomic<bool> exit_thread_flag = false;
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.
Moved initialization to "Initialize()" as suggestion does not compile.
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.
Ah, of course. It's atomic bool. My bad.
std::this_thread::sleep_for(std::chrono::milliseconds(100)); | ||
waited += 100; | ||
} | ||
if (!exit_thread_flag){ |
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.
Missing space flag) {
@@ -128,6 +137,8 @@ CdmAdapter::CdmAdapter( | |||
|
|||
CdmAdapter::~CdmAdapter() | |||
{ | |||
exit_thread_flag = true; | |||
std::this_thread::sleep_for(std::chrono::milliseconds(100)); |
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 value appears arbitrary. What exactly is this waiting for?
This value could very well work on one system but not on another. It would be better to continue once a condition is satisfied.
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 added another flag to check if timer thread is running or not. The sleep is/was for ensuring that the timer is not running anymore as otherwise cdm would get destroyed to early.
@@ -307,6 +318,8 @@ void CdmAdapter::CloseSession(uint32_t promise_id, | |||
const char* session_id, | |||
uint32_t session_id_size) | |||
{ | |||
exit_thread_flag = true; | |||
std::this_thread::sleep_for(std::chrono::milliseconds(100)); |
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 value appears arbitrary. What exactly is this waiting for?
This value could very well work on one system but not on another. It would be better to continue once a condition is satisfied.
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.
Same as above.
I rented a movie last night so can test. Only for next 48 hours tho I'll pull in your PR over latest Matrix branch and test |
@MisterD81 are you able to quickly run through what this pr is doing? Where does the sleep time come from? How does it know when its needed? Is this going to make all other content keep refreshing its license? Do we know why the code was commented out? I see its mostly uncommenting code so hard to tell what the missing pieces are |
@@ -307,6 +325,10 @@ void CdmAdapter::CloseSession(uint32_t promise_id, | |||
const char* session_id, | |||
uint32_t session_id_size) | |||
{ | |||
exit_thread_flag = true; | |||
while (timer_thread_running) { |
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.
Bracket on next line.
@@ -128,6 +140,10 @@ CdmAdapter::CdmAdapter( | |||
|
|||
CdmAdapter::~CdmAdapter() | |||
{ | |||
exit_thread_flag = true; | |||
while (timer_thread_running) { |
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.
Bracket on next line.
std::this_thread::sleep_for(std::chrono::milliseconds(100)); | ||
waited += 100; | ||
} | ||
if (!exit_thread_flag) { |
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.
Bracket on next line.
adp->TimerExpired(context); | ||
timer_thread_running = true; | ||
uint64_t waited = 0; | ||
while (!exit_thread_flag && delay > waited) { |
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.
Bracket on next line.
ok tested. without this PR - YT movie stopped around 4:50 for me with this PR, I see a license request every minute and it played pass 5minutes without issue @MisterD81 @anxdpanic |
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.
looks good and works good!
just need those formatting changes fixed that phunky pointed out.
And maybe squash down into a single commit :)
I'm also curious on these questions. |
I adjusted the formatting.
|
Thanks for explaining. Did you know there is actually a Timer class available as part or the add-on API? https://github.com/xbmc/xbmc/blob/master/xbmc/addons/kodi-dev-kit/include/kodi/tools/Timer.h So you can just implement ITimerCallback in your class and have any number of timers. |
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 contributing!
thanks for the explanation. I assume it was originally commented out due to the crashing issue that you fixed. @glennguy |
I am finding this fixes Binge (and possible other addons) that now requires license renewal even on non-Android platforms. |
…hile CloseSession is active to avoid endless loop. Sometimes when stopping a playback Kodi crashes with a following backtrace: (__m=<optimized out>, this=<optimized out>) at /usr/include/c++/12.1.0/bits/atomic_base.h:486 __b = <optimized out> at /usr/include/c++/12.1.0/atomic:87 (adp=std::shared_ptr<media::CdmAdapter> (use count 1, weak count 1) = {...}, delay=<optimized out>, context=<optimized out>) at /usr/src/debug/kodi-addon-inputstream-adaptive/inputstream.adaptive-20.3.3-Nexus/wvdecrypter/cdm/media/cdm/cdm_adapter.cc:81 at /usr/include/c++/12.1.0/bits/invoke.h:96, unsigned long long, void*), std::shared_ptr<media::CdmAdapter>, long long, void*> >::_M_invoke<0u, 1u, 2u, 3u>(std::_Index_tuple<0u, 1u, 2u, 3u>) (this=<optimized out>) at /usr/include/c++/12.1.0/bits/std_thread.h:252 at /usr/include/c++/12.1.0/bits/std_thread.h:259 at /usr/include/c++/12.1.0/bits/std_thread.h:210 at /usr/src/debug/gcc/libstdc++-v3/src/c++11/thread.cc:82 ret = <optimized out> pd = 0x85cf9080 unwind_buf = {cancel_jmp_buf = {{jmp_buf = {857629366, 66004830, -2049994624, -2008033272, -2008033282, 338, -2008033281, 8387456, -2058383360, -2049995908, 1920, 1080, 3840, 2160, 0, 10000000, -1431355392, 1107558643, 0 <repeats 46 times>}, mask_was_saved = 0}}, priv = {pad = {0x0, 0x0, 0x0, 0x0}, data = {prev = 0x0, cleanup = 0x0, canceltype = 0}}} not_first_call = <optimized out> robust = <optimized out> After searching in the github history it came out that there was an idenctical issue adressed in xbmc#589. For some reason that piece of code got removed in xbmc@8889fe9 (in pull request xbmc#883). The stack trace is from the Raspberry Pi 4 machine. But I can also reproduce the issue on my x86_64 Linux machine by adding a 1s sleep in the beginning of CdmAdapter::SetTimer. In order to reproduce it is necessary to quickly play/stop streams. Following script can be used to ease the reproduction. In my environment when running following script for 1-3 minutes the screen freezes because of the busy loop mentioned in xbmc#589. After applying mentioned commit I'm not able to reproduce the issue anymore. ``` set -x URI='Change it to some uri to play' while true; do curl -s "http://kodi:[email protected]:8080/jsonrpc" -H 'Content-Type: application/json' --data "{\"jsonrpc\":\"2.0\",\"method\":\"Player.Open\",\"params\":{\"item\":{\"file\":\"$URI\"}}}" sleep $[ ($RANDOM % 70 + 30) / 10.0 ] curl -s "http://kodi:[email protected]:8080/jsonrpc" -H 'Content-Type: application/json' --data '{"jsonrpc":"2.0","method":"Player.Stop","params":{"playerid":1}}' sleep 1 done ```
At least purchased movies in Youtube addon stopped working. Always after 5 minutes we can see in the logs that there is no decryption key. Surprisingly I was only able to get this license behavior only on Windows and Linux ARM. On Linux x64 the license renewal was not required.
See the corresponding bug in YT addon: anxdpanic/plugin.video.youtube#35
I figured out with EME logger of Chrome that the CDM is sending periodically a session message to renew the license.
IAS is not doing that at the moment which results in the behavior that playback always stops after 5 minutes. I found in the code the commented-out parts for the license renewal and activated them. The additional adjustments were required to ensure that the thread is stopped correctly. Otherwise Kodi crashes when the playback is finished or is stopped.
We can argue about the sleep duration, but I could not observe a high CPU increase or high delay when playback stopped with the 100ms.
Since I'm still on Leia this patch is only created based on the Leia PR (#588)