Skip to content

session: fix a deadlock due to onPlayRequested() #2256

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

nift4
Copy link

@nift4 nift4 commented Mar 22, 2025

Issue: #1197

@nift4
Copy link
Author

nift4 commented May 16, 2025

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 :)

@nift4
Copy link
Author

nift4 commented Jul 13, 2025

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!

@marcbaechinger
Copy link
Contributor

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:

  • A unit test that reproduces the bug you are fixing. This is crucial for me to understand the problem and verify the solution.
  • A brief explanation of the reasoning behind your design choices.
  • Code that is consistent with the existing Media3 coding style.

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.

@nift4
Copy link
Author

nift4 commented Jul 25, 2025

Hi @marcbaechinger, sorry for flooding you with PRs :) I am not a professional so please excuse any stupid questions:

A unit test that reproduces the bug you are fixing. This is crucial for me to understand the problem and verify the solution.

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?

A brief explanation of the reasoning behind your design choices.

No problem, I'll do that.

Code that is consistent with the existing Media3 coding style.

If you mean this: https://github.com/androidx/media/blob/release/CONTRIBUTING.md#code-style
I generally try to format my PRs with google-java-format before uploading. I'll make sure I haven't forgotten it in any of the PRs.

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. :)

@marcbaechinger
Copy link
Contributor

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.

@nift4
Copy link
Author

nift4 commented Jul 25, 2025

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants