Skip to content
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

Fix DASH thumbnails cropping the incorrect tile for non-square images #1300

Merged
merged 6 commits into from
May 1, 2024
Merged

Fix DASH thumbnails cropping the incorrect tile for non-square images #1300

merged 6 commits into from
May 1, 2024

Conversation

hakonschia
Copy link
Contributor

The calculation for finding the column a tileIndex corresponds to is currently tileIndex % inputFormat.tileCountVertical. This should instead be tileIndex % inputFormat.tileCountHorizontal (see this SO answer) to work for non-square images. It works for square images since tileCountVertical == tileCountHorizontal, so the same values are produced.

Using tileCountVertical for non-square images results in either:

  • When tileCountVertical is equal to 1 (sprites with one row of images) it always crops out the first tile
  • When tileCountVertical is larger than 1 it will attempt to crop out a bitmap outside the bounds of the larger bitmap, resulting in Bitmap.createBitmap() throwing an exception.
Exception thrown 
Playback error
  androidx.media3.exoplayer.ExoPlaybackException: Unexpected runtime error
   at androidx.media3.exoplayer.ExoPlayerImplInternal.handleMessage(ExoPlayerImplInternal.java:703)
   at android.os.Handler.dispatchMessage(Handler.java:103)
   at android.os.Looper.loopOnce(Looper.java:232)
   at android.os.Looper.loop(Looper.java:317)
   at android.os.HandlerThread.run(HandlerThread.java:85)
 Caused by: java.lang.IllegalArgumentException: x + width must be <= bitmap.width()
   at android.graphics.Bitmap.createBitmap(Bitmap.java:910)
   at android.graphics.Bitmap.createBitmap(Bitmap.java:869)
   at androidx.media3.exoplayer.image.ImageRenderer.cropTileFromImageGrid(ImageRenderer.java:575)
   at androidx.media3.exoplayer.image.ImageRenderer.drainOutput(ImageRenderer.java:343)
   at androidx.media3.exoplayer.image.ImageRenderer.render(ImageRenderer.java:182)
   at androidx.media3.exoplayer.ExoPlayerImplInternal.doSomeWork(ExoPlayerImplInternal.java:1114)
   at androidx.media3.exoplayer.ExoPlayerImplInternal.handleMessage(ExoPlayerImplInternal.java:546)
   at android.os.Handler.dispatchMessage(Handler.java:103) 
   at android.os.Looper.loopOnce(Looper.java:232) 
   at android.os.Looper.loop(Looper.java:317) 
   at android.os.HandlerThread.run(HandlerThread.java:85) 

I have updated ImageRenderTest to use a 2x3 image instead of 2x2. Reverting my change will make the updated tests fail, as expected, but please verify they still work as intended.

I have created a separate branch that displays thumbnails in the demo app that can be used for testing. Two samples are added, one for the one row case and one for non-squares with more than one row. The videos below are taken from this branch.

Before
Screen_recording_20240420_085532.mp4
After
Screen_recording_20240420_085434.mp4

@microkatz
Copy link
Contributor

microkatz commented Apr 25, 2024 •
edited
Loading

@hakonschia

Thank you for identifying the issue and providing a pull request to remedy it. Your solution makes sense to me. All that I ask is if you can just create an individual unit test and sample with the six frames rather than altering the current tests. That would be great!

@hakonschia
Copy link
Contributor Author

@microkatz I've created a new test now instead, but I'm not really if what I did with overridenBitmap is the best way of using a different bitmap in this test

Let me know if you'd prefer to have these two commits squashed to avoid ImageRenderTest having changes in the first commit that were just reverted again in the second commit

@microkatz
Copy link
Contributor

microkatz commented Apr 30, 2024 •
edited
Loading

@hakonschia

Ah. I see the current issue of how its setup in setup with the fakeDecoderFactory instance providing either fakeDecodedBitmap1 or fakeDecodedBitmap2. I'm not sure I like the overridenBitmap. Instead of having the overridenBitmap, how about you just reassign and reinitalize the renderer in your singular unit test?

@hakonschia
Copy link
Contributor Author

@microkatz sure, that seems like a better solution to me, I've pushed that change now. I just copied queuingImageOutput, but it might make more sense for this to be a moved to a class variable at this point instead of duplicating it

@microkatz microkatz force-pushed the fix-dash-thumbnails-cropping branch from 1d242bd to ddbb0ef Compare May 1, 2024 11:21
@microkatz
Copy link
Contributor

I'm going to send this for internal review now. You may see some more commits being added as I make changes in response to review feedback. Please refrain from pushing any more substantive changes as it will complicate the internal review - thanks!

hakonschia
hakonschia commented May 1, 2024
View reviewed changes
RELEASENOTES.md
Comment on lines +43 to +49
* Add support for non-square DASH thumbnail grids
([#1255](https://github.com/androidx/media/pull/1300)).
Copy link
Contributor Author

@hakonschia hakonschia May 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@microkatz The PR number for the link text doesn't match the actual PR (and issue 1255 is unrelated to this PR)

@microkatz microkatz force-pushed the fix-dash-thumbnails-cropping branch from e5a30d4 to 420c58b Compare May 1, 2024 15:13
hakonschia and others added 6 commits May 1, 2024 16:26
@hakonschia @microkatz
Use inputFormat.tileCountHorizontal to calculate tileStartXCoordinate…
9ced27a 
… for cropping the correct tile from outputBitmap

To find the column of an index in a matrix the formula "column = index % width" should be used, not "column = index % height"

If inputFormat.tileCountVertical was equal to 1 then it would not throw an error, but instead result in the first tile of the bitmap always being returned. If inputFormat.tileCountVertical was larger than 1 then Bitmap.createBitmap() would throw an error as it would attempt to go outside the bounds of outputBitmap

ImageRenderTest has been updated to test for 2x3 images so that tileCountVertical != tileCountHorizontal. These tests passed previously because they were equal, so using tileCountVertical produced the same results as tileCountHorizontal
@hakonschia @microkatz
Revert changes in ImageRenderTest from 3a030da and create a new unit …
e27f527 
…test for non square images instead
@hakonschia @microkatz
Create new ImageRenderer in render_tiledImageNonSquare_rendersAllImag…
cf41ba3 
…esToOutput to allow it to use a separate bitmap from the other tests
@microkatz
Format with google-java-format
6d62d34
@microkatz
Add release note for DASH non-square thumbnail grid fix
690bb1a
@microkatz
Updated non-Square thumbnail unit test to contain non-Square thumbnails
acc5a3b
@microkatz microkatz force-pushed the fix-dash-thumbnails-cropping branch from 420c58b to acc5a3b Compare May 1, 2024 16:27
@copybara-service copybara-service bot merged commit d385072 into androidx:main May 1, 2024
1 check passed
@icbaker icbaker mentioned this pull request May 30, 2024
Closed
1 task
@androidx androidx locked and limited conversation to collaborators Jul 9, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees

@microkatz microkatz

Labels
should merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@hakonschia @microkatz