session: fix a deadlock due to onPlayRequested()#2256
session: fix a deadlock due to onPlayRequested()#2256nift4 wants to merge 2 commits intoandroidx:mainfrom
Conversation
|
Hi @marcbaechinger, it's only been 3 weeks since the last ping (sorry if I am bothering you) but considering you are active rn, maybe you could add this PR to your review queue? thanks alot :) |
|
Hi @marcbaechinger, sorry for repeatedly pinging, could I convince you to review this PR? I've shipped it in production for about two months already with only one minor issue (which I have already fixed). It's been working pretty well. Would be appreciated! |
|
Thanks for your contributions! To help me review your pull requests more effectively, I'd like to outline what I'll be looking for in each submission:
My review will also verify that the changes are efficient, work correctly with older versions of Media3 and platform APIs, and that any API changes include a backward-compatible migration path. While it's great that you are already using these changes in your app, we still need this comprehensive test coverage to proceed with our internal review process. I plan to start looking at this PR this week or next. Providing the information above will make the review process much smoother and help me reach a decision on your contribution more quickly. |
|
Hi @marcbaechinger, sorry for flooding you with PRs :) I am not a professional so please excuse any stupid questions:
Sure, I'll add a unit test to this PR and #2626. For the other PRs assigned to you (#2642, #2643, #2647), they only make things faster. I'm not sure how to add a unit test there. Do you want me to add some to these too? If so, could you please give me a small hint how those are supposed to work?
No problem, I'll do that.
If you mean this: https://github.com/androidx/media/blob/release/CONTRIBUTING.md#code-style If you're talking about coding style on a more general level (word choice in variable names, methods used for switching threads, etc), I'm trying my best to adapt to the code around me, but please do tell me if I do anything wrong. :) |
|
Never mind the point regarding code style. I haven't meant all these point are not met by your code and I can imagine it's not always easy to know what it needs. This is just a general statement around what I have to do for making a PR ready for internal code review and everything that deviates from that means that it may take a certain time to adjust it. |
|
I've added a unit test to my PR that fails before my fix and passes after my fix. For brief explaination of the fix, see: #1197 (comment) |
|
Rebased & solved conflicts |
503e42c to
4e78edb
Compare
|
Hi @marcbaechinger, please could you check this PR? It is now over 1 year old. In order to be able to use a player with non-main thread I currently have to carry three or so bugfix patches to the session library (of which this is the first one - though #2499 being fixed is a great start already), which I'd like to get upstreamed (in order to discuss whether it is the correct way and to have the library bug solved for everyone, but also in order to not need to rebase frequently). I'm willing to write tests and I did so here, so I hope you can take a look. Thank you very much! |
|
This deadlock was fixed by commit c8d9a09 so I'm closing the PR |
Issue: #1197