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.

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

Full screen bitmap smpte-tt subtitle not rendered correctly #5633

Closed
szaboa opened this issue Mar 14, 2019 · 18 comments
Closed

Full screen bitmap smpte-tt subtitle not rendered correctly #5633

szaboa opened this issue Mar 14, 2019 · 18 comments
Assignees
Labels

Comments

@szaboa
Copy link
Contributor

szaboa commented Mar 14, 2019

[REQUIRED] Issue description

Support for image based SMPTE-TT subtitles was implemented in #1583, however it's not working quite well for "full screen" subtitles as it renders these out of the screen and with wrong height. By full screen I mean when the subtitle should take size of the surface. This is the case when tt tts:extent = region tts:extent in case of pixel based regions.

[REQUIRED] Reproduction steps

It can be reproduced with the ExoPlayer demo app, just needs a stream with image based SMPTE-TT subtitle track where the Base64 encoded bitmap is "full screen".

[REQUIRED] Link to test content

Email

[REQUIRED] A full bug report captured from the device

No logs required.

[REQUIRED] Version of ExoPlayer being used

ExoPlayer 2.9.3 - 2.9.6

[REQUIRED] Device(s) and version(s) of Android being used

It can be reproduced every time and not device dependent.
In my tests, I've used Pixel 1.

@szaboa
Copy link
Contributor Author

szaboa commented Mar 14, 2019

Example:
The manifest contains in tt tag let's say tts:extent="720px 576px" and there are regions like <region xml:id="region_0" tts:extent="720px 576px" tts:origin="0px 0px"/>.

Two problems causing this issue:

1. Incorrect height calculation
In the example above the bitmap will take the width of the parent correctly, but the height is scaled up to keep the bitmap's aspect ration.

Height calculation in com.google.android.exoplayer2.ui.SubtitlePainter#setupBitmapLayout:

int height = cueBitmapHeight != Cue.DIMEN_UNSET ? Math.round(parentHeight * cueBitmapHeight)
        : Math.round(width * ((float) cueBitmap.getHeight() / cueBitmap.getWidth()));

The cueBitmapHeight will be Cue.DIMEN_UNSET because that's what we pass in com.google.android.exoplayer2.text.ttml.TtmlNode#getCues (line 238).

This logic is OK for non-full screen subtitles, but for a full screen one it doesn't force the bitmap to fit the parent size. This results incorrect positioning.

Inside com.google.android.exoplayer2.text.ttml.TtmlDecoder#parseRegionAttributes the width and height calculations are OK and both of them are 1 (which means 100% width and 100% height). But the height is not passed to the TtmlRegion.

Proposed solution:

Pass the height from parseRegionAttributes to TtmlRegion and inside com.google.android.exoplayer2.text.ttml.TtmlNode#getCues pass this height to Cue (line 238) or pass it only if both of the region's width and height is 1, this would only affect the full screen bitmaps. In this way the bitmap's height would be set to the parent height in com.google.android.exoplayer2.ui.SubtitlePainter#setupBitmapLayout.

2. Incorrect position anchor

Inside com.google.android.exoplayer2.text.ttml.TtmlNode#getCues we pass Cue.ANCHOR_TYPE_MIDDLE as a default position anchor for image based subtitles. This will render a full screen image based subtitle offscreen (top half of the bitmap is outside of the view).

Again, because the y calculation in com.google.android.exoplayer2.ui.SubtitlePainter#setupBitmapLayout:

int y = Math.round(cuePositionAnchor == Cue.ANCHOR_TYPE_END ? (anchorY - height)
        : cuePositionAnchor == Cue.ANCHOR_TYPE_MIDDLE ? (anchorY - (height / 2)) : anchorY);

Proposed solution:

Change the default position anchor to Cue.ANCHOR_TYPE_START or do not set anything, this way it will fall back to anchor start.

@ojw28
What do you think? Once we agree on a solution, I'll fix this.

@szaboa
Copy link
Contributor Author

szaboa commented Mar 27, 2019

Can someone have a look on this please?

@AquilesCanta
Copy link
Contributor

@szaboa, have you shared a stream with us? We will need sample media before we look into this. If you think the solution is straightforward, you might want to send a pull request directly: But most likely, we will still need a sample stream.

@szaboa
Copy link
Contributor Author

szaboa commented Mar 28, 2019

I will try to get a sample stream as soon as I can.

Actually, I was who implemented the support for image based subtitles (smpte-tt) and I didn't pay enough attention to this use case. That's why I proposed the fix.

@szaboa szaboa closed this as completed Mar 28, 2019
@szaboa
Copy link
Contributor Author

szaboa commented Mar 28, 2019

Sorry, accidentally closed the issue, miss click :)

@szaboa
Copy link
Contributor Author

szaboa commented Apr 1, 2019

@AquilesCanta, sorry for the late response :)

Updated ticket's description with sample streams. During testing make sure you pick the second Norwegian subtitle track and note that you might need to wait a little for the subs to show up (probably not synced correctly in this test content).

I've also created a pull request with the proposed solution (#5711).


And to make sure nothing changed for regular (not full screen) image based subtitles, I've also tested with this sample stream (which I used as a reference when I implemented support for image based subs at first place): https://livesim.dashif.org/dash/vod/testpic_2s/img_subs.mpd

Screen Shot 2019-04-01 at 5 27 01 PM


@ojw28, you might want to have a look too on the proposed solution, as we have discussed this feature quite a lot at the beginning :)

@szaboa
Copy link
Contributor Author

szaboa commented Apr 25, 2019

Any update on this? :)

@szaboa
Copy link
Contributor Author

szaboa commented May 17, 2019

@ojw28, I would really appreciate if someone could take a look on this.

@AquilesCanta
Copy link
Contributor

I'll look into it this week.

@szaboa
Copy link
Contributor Author

szaboa commented May 21, 2019

Thank you!

@AquilesCanta
Copy link
Contributor

Hi, I'm getting a 404 now. Can you please make sure the streams are available?

@szaboa
Copy link
Contributor Author

szaboa commented May 24, 2019

Hi, will send the new ones via email shortly.
Edit. I will be able to send it only on Monday (27.05).

@AquilesCanta
Copy link
Contributor

Ok. Please post below once you do. I suppose I'll get to this on Tuesday.

@szaboa
Copy link
Contributor Author

szaboa commented May 24, 2019

@AquilesCanta
Ok, so no need for streams actually. I've created a TTML subtitle file:
https://drive.google.com/uc?export=download&id=1Rf7sWPgnDs7Ax7XOcFlNXwu2dNj8s8Pv

Which contains a cue that is full screen (big red rectangle) and it should cover the whole screen, as it has a region 1280px 720px and the root tt value is also 1280px 720px (which gives the window reference).

Please side load this subtitle to any of the streams inside the exo's demo application.

Before fix:

before_fix

After fix:

after_fix

@AquilesCanta
Copy link
Contributor

We will be pushing a fix soon. Please try it out once a commit appears referenced below so that you can make sure it works before it reaches a release version.

@szaboa
Copy link
Contributor Author

szaboa commented May 28, 2019

Alright, thanks.

tonihei pushed a commit that referenced this issue May 30, 2019
According to Cue's constructor (for bitmaps) documentation:
+ cuePositionAnchor does horizontal anchoring.
+ cueLineAnchor does vertical anchoring.

Usage is currently inverted.

Issue:#5633
PiperOrigin-RevId: 250253002
tonihei pushed a commit that referenced this issue May 30, 2019
+ Use start for anchoring, instead of center.
+ Add the height to the TTML bitmap cue rendering layout.

Issue:#5633
PiperOrigin-RevId: 250519710
@szaboa
Copy link
Contributor Author

szaboa commented May 31, 2019

@AquilesCanta
The fix seems to be working fine, thank you 👍

@AquilesCanta
Copy link
Contributor

Glad to hear that. Let me know if you run into any other issues.

ojw28 pushed a commit that referenced this issue Jun 3, 2019
According to Cue's constructor (for bitmaps) documentation:
+ cuePositionAnchor does horizontal anchoring.
+ cueLineAnchor does vertical anchoring.

Usage is currently inverted.

Issue:#5633
PiperOrigin-RevId: 250253002
ojw28 pushed a commit that referenced this issue Jun 3, 2019
+ Use start for anchoring, instead of center.
+ Add the height to the TTML bitmap cue rendering layout.

Issue:#5633
PiperOrigin-RevId: 250519710
@google google locked and limited conversation to collaborators Oct 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

4 participants