Fix ALPHA_8 format support for bitmap in Cue. #2058
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
peerless2012
changed the title
| @@ -835,6 +837,9 @@ public Cue build() { | |||
| private static final String FIELD_MULTI_ROW_ALIGNMENT = Util.intToStringMaxRadix(2); | |||
| private static final String FIELD_BITMAP_PARCELABLE = Util.intToStringMaxRadix(3); | |||
| private static final String FIELD_BITMAP_BYTES = Util.intToStringMaxRadix(18); | |||
| private static final String FIELD_BITMAP_ORIGIN_WIDTH = Util.intToStringMaxRadix(19); | |||
There was a problem hiding this comment.
what is the meaning of the word 'origin' here?
There was a problem hiding this comment.
The bitmap compress don't support ALPHA_8, so we have to store the bitmap original size and buffer, then when read cue, we need use the bitmap size.
There was a problem hiding this comment.
The bitmap compress don't support ALPHA_8, so we have to store the bitmap original size and buffer, then when read cue, we need use the bitmap size.
There was a problem hiding this comment.
I understand why you need to transfer the size and width separately to the bytes. I don't understand what the meaning of the word 'origin' is. My first assumption is that this means the (0,0) point: https://en.wikipedia.org/wiki/Origin_(mathematics), but I'm not sure that really makes sense. So what is the intended meaning?
|
|
||
| /** Tests for {@link Cue}. */ | ||
| @RunWith(AndroidJUnit4.class) | ||
| public class CueTest { |
There was a problem hiding this comment.
let's just move the whole of CueTest to androidTest instead of splitting it like this.
There was a problem hiding this comment.
The robolectric test suite not support ALPHA_8 bitmap, so we have to use AndroidTest.
See Request support bitmap with ALPHA_8.
There was a problem hiding this comment.
I understand.
I'm asking you to move the existing CueTest from libraries/common/src/test/java/androidx/media3/common/text to libraries/common/src/androidTest/java/androidx/media3/common/text and then add this new test method to it.
The current change results in two classes called androidx.media3.common.text.CueTest which results in undefined behaviour when debugging in Android Studio, and having to look in two places when checking what parts of Cue are tested.
| bitmap = Bitmap.createBitmap(bitmapWidth,bitmapHeight, Bitmap.Config.ALPHA_8); | ||
| Buffer buffer = ByteBuffer.wrap(bitmapBytes); | ||
| bitmap.copyPixelsFromBuffer(buffer); | ||
| builder.setBitmap(bitmap); |
There was a problem hiding this comment.
optional: de-duplicate the 3 builder.setBitmap lines, so the code is instead:
@Nullable Bitmap bitmap = bundle.getParcelable(FIELD_BITMAP_PARCELABLE);
if (bitmap == null) {
// handle bytes case (both compressed and uncompressed) with
// existing code, but assigning `bitmap` each time instead of setting
// it on the builder.
}
builder.setBitmap(bitmap); // Passing null is fine here
Bitmap.compress not support ALPHA_8 format, so we just copy the pixel buffer.
This should fix #2054