8000 Introduce HAMediaPlayer to migrate existing player to compose by TimoPtr · Pull Request #5408 · home-assistant/android · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Introduce HAMediaPlayer to migrate existing player to compose #5408

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 14 commits into from
Jun 16, 2025
Merged

Introduce HAMediaPlayer to migrate existing player to compose #5408

merged 14 commits into from
Jun 16, 2025

Conversation

TimoPtr
Copy link
Collaborator
@TimoPtr TimoPtr commented Jun 11, 2025 •
edited
Loading

Summary

In order to migrate to compose the WebViewActivity we need to have a compose view of the media player. By default only the surface is provided not the control, this PR adds a very basic implementation of the controls. It is not feature complete. Since the WebView at the moment is only using this player to play a camera, the implementation is currently limited to this use case.

I decided to first make a PR that replace the player in DevPlayground so it can be use later in the PR for the WebView to compose.

Checklist

  • New or updated tests have been added to cover the changes following the testing guidelines.
  • The code follows the project's code style and best_practices.
  • The changes have been thoroughly tested, and edge cases have been considered.
  • Changes are backward compatible whenever feasible. Any breaking changes are documented in the changelog for users and/or in the code for developers depending on the relevance.

Screenshots

demo_player.mp4

Any other notes

I'm not a compose expert so any comment on how to make it better is very much appreciated.
I added the media session so that the system can control the player too. It is useful to stop a playback for instance when the player is in a PiP mode.

github-advanced-security[bot]
github-advanced-security bot found potential problems Jun 11, 2025
View reviewed changes
@TimoPtr TimoPtr marked this pull request as ready for review June 11, 2025 14:05
@jpelgrom
Copy link
Member

I think this would qualify as visual changes not in the frontend, so please include a screenshot in the future for similar changes ;)

@TimoPtr
Copy link
Collaborator Author
TimoPtr commented Jun 11, 2025

I will the only thing for now is that until it is not integrated in the app it's not very clear how it looks like. I'm going to add how it looks like on the dev activity.

jpelgrom reviewed Jun 11, 2025
View reviewed changes
Copy link
Member
@jpelgrom jpelgrom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like a pretty good start for a basic player!

If I turn off the screen, I lose video progress and it starts back at the start of the video. Not entirely sure if that is the demo video or the Compose implementation.

Small size player highlights layout issues:

  • the bottom bar partially obscures the play/pause icon
  • start and end padding isn't the same because of the icon button, I think the text needs more padding at the start

image

LeakCanary popped up after exiting the activity, saying there was an issue:

LeakCanary trace 
┬───
│ GC Root: System class
│
├─ android.provider.FontsContract class
│    Leaking: NO (HomeAssistantApplication↓ is not leaking and a class is never
│    leaking)
│    ↓ static FontsContract.sContext
├─ io.homeassistant.companion.android.HomeAssistantApplication instance
│    Leaking: NO (Application is a singleton)
│    mBase instance of android.app.ContextImpl
│    ↓ Application.mLoadedApk
│                  ~~~~~~~~~~
├─ android.app.LoadedApk instance
│    Leaking: UNKNOWN
│    Retaining 10.1 kB in 162 objects
│    mApplication instance of io.homeassistant.companion.android.
│    HomeAssistantApplication
│    Receivers
│    ..DemoExoPlayerActivity@38767496
│    ....MediaSessionLegacyStub$MediaButtonReceiver@39579224
│    ..HomeAssistantApplication@33649392
│    ....MediaPlayerControlsWidget@35483216
│    ....SensorReceiver@35328608
│    ....VisibilityTracker@34591576
│    ....cs@39014000
│    ....TemplateWidget@35487080
│    ....io@39255048
│    ....ButtonWidget@35467640
│    ....WebsocketBroadcastReceiver@35102864
│    ....TodoWidget@35494184
│    ....it@39172856
│    ....NetworkTypeObserver$Receiver@39433520
│    ....EntityWidget@35474104
│    ....ik@39255632
│    ↓ LoadedApk.mReceivers
│                ~~~~~~~~~~
├─ android.util.ArrayMap instance
│    Leaking: UNKNOWN
│    Retaining 9.6 kB in 148 objects
│    ↓ ArrayMap.mArray
│               ~~~~~~
├─ java.lang.Object[] array
│    Leaking: UNKNOWN
│    Retaining 9.5 kB in 146 objects
│    ↓ Object[0]
│            ~~~
╰→ io.homeassistant.companion.android.developer.DemoExoPlayerActivity instance
​     Leaking: YES (ObjectWatcher was watching this because io.homeassistant.
​     companion.android.developer.DemoExoPlayerActivity received
​     Activity#onDestroy() callback and Activity#mDestroyed is true)
​     Retaining 280.9 kB in 4236 objects
​     key = 6f808c2c-a2cc-4915-8dc3-e1ac7f0be014
​     watchDurationMillis = 50022
​     retainedDurationMillis = 45018
​     mApplication instance of io.homeassistant.companion.android.
​     HomeAssistantApplication
​     mBase instance of androidx.appcompat.view.ContextThemeWrapper

METADATA

Build.VERSION.SDK_INT: 36
Build.MANUFACTURER: Google
LeakCanary version: 2.14
App process name: io.homeassistant.companion.android.debug
Class count: 41689
Instance count: 274500
Primitive array count: 197937
Object array count: 40711
Thread count: 50
Heap total bytes: 36924602
Bitmap count: 3
Bitmap total bytes: 36163
Large bitmap count: 0
Large bitmap total bytes: 0
Db 1: open /data/user/0/io.homeassistant.companion.android.
debug/no_backup/androidx.work.workdb
Db 2: open /data/user/0/io.homeassistant.companion.android.
debug/databases/HomeAssistantDB
Stats: LruCache[maxSize=3000,hits=133643,misses=254955,hitRate=34%]
RandomAccess[bytes=12572701,reads=254955,travel=136972692539,range=45653516,size
=56454593]
Analysis duration: 79901 ms

I learned some new KDoc syntax, nice.

@TimoPtr
Copy link
Collaborator Author
TimoPtr commented Jun 12, 2025

About the Leak did it happen with PiP? Are you able to reproduce it all the time?

@TimoPtr
Copy link
Collaborator Author
TimoPtr commented Jun 12, 2025

On small layout the current layout (from media3-ui) hide the time and display only the progress
image

I decided to hide the bottom controls when there is not enough space for it.

To handle

If I turn off the screen, I lose video progress and it starts back at the start of the video. Not entirely sure if that is the demo video or the Compose implementation.

I would be in favor of introducing a viewModel. I didn't here but it's getting more and more complicated and a VM would make it cleaner. I don't think we need that for the first impl for the WebView.

@TimoPtr TimoPtr requested review from jpelgrom and dshokouhi June 12, 2025 13:50
@TimoPtr 8000
Copy link
Collaborator Author
TimoPtr commented Jun 12, 2025 •
edited
Loading

While working on the migration of the WebViewActivity I came around the fact that I should have extract the Player initialization out of the Compose definition. 🤦🏻

@github-actions GitHub Actions
Copy link
github-actions bot commented Jun 12, 2025 •
edited
Loading

Test Results

 41 files   41 suites   4m 47s ⏱️
130 tests 130 ✅ 0 💤 0 ❌
140 runs  140 ✅ 0 💤 0 ❌

Results for commit aa449de.

♻️ This comment has been updated with latest results.

@jpelgrom
Copy link
Member 8000

About the Leak did it happen with PiP? Are you able to reproduce it all the time?

I was able to reproduce it all the time, but not anymore since you removed the MediaSession. Just opening the activity, and going back using the system back button was enough to trigger it.

On small layout the current layout (from media3-ui) hide the time and display only the progress
I decided to hide the bottom controls when there is not enough space for it.

OK, seems reasonable. New issue that also would apply to the classic view ExoPlayer: the preview in the playground activity can no longer be full screened, maybe it's better to make that one bigger so we can test all functionality (making it smaller is easy enough with splitscreen).

I don't think we need that for the first impl for the WebView.

OK, let's keep that out of scope for this PR.

jpelgrom
jpelgrom reviewed Jun 12, 2025
View reviewed changes
@TimoPtr TimoPtr requested a review from jpelgrom June 13, 2025 07:08
jpelgrom
jpelgrom approved these changes Jun 15, 2025
View reviewed changes
Copy link
Member
@jpelgrom jpelgrom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems good enough for now, I couldn't discover any more issues. We can improve the UI in the future based on feedback.

@TimoPtr TimoPtr enabled auto-merge (squash) June 16, 2025 07:56
@TimoPtr TimoPtr merged commit 41d80cc into main Jun 16, 2025
19 checks passed
@TimoPtr TimoPtr deleted the feature/media3-compose branch June 16, 2025 08:20
@TimoPtr TimoPtr mentioned this pull request Jun 17, 2025
Open
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jpelgrom jpelgrom jpelgrom approved these changes

@dshokouhi dshokouhi Awaiting requested review from dshokouhi

Assignees
No one assigned
Labels
cla-signed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@TimoPtr @jpelgrom
0