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

Introduce edgeLayout to keep outline color #6724

Merged
merged 1 commit into from
Jan 30, 2020

Conversation

nnoury
Copy link
Contributor

@nnoury nnoury commented Dec 4, 2019

Fix outline color style in subtitles

When a ForegroundColorSpan changes the foreground color, it is also applied
to the outline painter. In order to keep the correct color, one needs to
filter out theses span. We do this with a new cue that is our text source
for the specific edgeLayout.

Test: Manual check - Subtitle view can show custom color subtitles from specific Subtitle
Renderer and outline is shown correctly using user defined color.

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@nnoury
Copy link
Contributor Author

nnoury commented Dec 4, 2019

@googlebot I signed it!

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@ojw28
Copy link
Contributor

ojw28 commented Dec 6, 2019

@icbaker - Please take a look. Thanks!

@icbaker
Copy link
Collaborator

icbaker commented Dec 9, 2019

Thanks for the contribution!

I'm afraid I'm not 100% clear what bug/problem you're solving here - would it be possible to provide repro instructions and/or test media that demonstrates the problem?

You can email test media to dev.exoplayer@gmail.com if you're unable to share it directly on github - make sure the subject is Issue #6724.

@nnoury
Copy link
Contributor Author

nnoury commented Dec 10, 2019

To reproduce, and see that the outline is painted with the foreground color:

either manually change user captionning style
Launch Captionning manager,
am start -a android.settings.CAPTIONING_SETTINGS

switch caption style to custom (last row radio button),
then select Edge Type -> Outline
on tv, enable then disable Show background to workaround an initialization value bug in leanback, on phones select no background
then back back back

or you can programmatically do (my codebase is in Kotlin)

    val captionStyle = CaptionStyleCompat(Color.WHITE, Color.TRANSPARENT, Color.TRANSPARENT,
            EDGE_TYPE_OUTLINE, Color.BLACK, Typeface.DEFAULT)
    subtitleView.setStyle(captionStyle)

in library/ui/src/main/java/com/google/android/exoplayer2/ui/PlayerView.java:449

Test in exoplayer demo in Subtitles -> WebVTT with the following patch applied:

diff --git a/demos/main/src/main/assets/media.exolist.json b/demos/main/src/main/assets/media.exolist.json
index 06f063b1c..2faba848b 100644
--- a/demos/main/src/main/assets/media.exolist.json
+++ b/demos/main/src/main/assets/media.exolist.json
@@ -601,6 +601,13 @@
   {
     "name": "Subtitles",
     "samples": [
+        {
+        "name": "WebVTT",
+        "uri": "https://nnoury.github.io/sintel-short.mp4",
+        "subtitle_uri": "https://nnoury.github.io/sintel-en.vtt",
+        "subtitle_mime_type": "text/vtt",
+        "subtitle_language": "en"
+      },
       {
         "name": "TTML",
         "uri": "https://html5demos.com/assets/dizzy.mp4",

@nnoury
Copy link
Contributor Author

nnoury commented Dec 10, 2019

Before apply MR:
Screenshot_1575988264
After:
Screenshot_1575988331

@icbaker
Copy link
Collaborator

icbaker commented Dec 11, 2019

Thanks for the clear repro instructions & screenshots - I can see the problem you're fixing here.

I think what we've got so far isn't quite right - or rather doesn't interact well with existing decisions. With your change and a non-transparent backgroundColor It seems the background is drawn above the outline & below the text - you can see in the screenshot a bit of the w's edge sticking out the side of the box. Seems like the background should be behind the outline?

CaptionStyleCompat outlineWithBackgroundNoWindow =
          new CaptionStyleCompat(
              /* foregroundColor= */ Color.WHITE,
              /* backgroundColor= */ Color.CYAN,
              /* windowColor= */ Color.TRANSPARENT,
              CaptionStyleCompat.EDGE_TYPE_OUTLINE,
              /* edgeColor= */ Color.BLACK,
              /* typeface= */ null);
subtitleView.setStyle(outlineWithBackgroundNoWindow);

Screenshot_20191211-165827

Can you have a go at seeing if you can get the edge to appear in front of the background?

I think it's happening because the backgroundColor is rendered using a BackgroundColorSpan on cueText. I guess this should be on cueTextEdge (only) if edgeType is OUTLINE or RAISED.

This might be a bit tricky in the current code layout - if there's an obvious way to reshuffle things a bit to make it much better feel free to have a go.

@google-oss-bot
Copy link
Collaborator

Hey @nnoury. We need more information to resolve this issue but there hasn't been an update in 14 days. I'm marking the issue as stale and if there are no new updates in the next 7 days I will close it automatically.

If you have more information that will help us get to the bottom of this, just add a comment!

@ojw28
Copy link
Contributor

ojw28 commented Jan 2, 2020

Removing labels that might cause the bot to close this. @nnoury - Are you going to be able to take a look at the comments from @icbaker above?

@phhusson
Copy link
Contributor

Quick update: we initially thought that the issue raised would need to add yet another painter, and that it would raise other issues, but we checked again and now it seems pretty straightforward.

We'll try reporting back soon.

Fix outline style subtitle

When a ForegroundColorSpan changes the foreground color, it is also applied
to the outline painter. In order to keep the correct color, one needs to
filter out theses span. We do this with a new cue that is our text source
for the specific edgeLayout.

Take care to only apply background color once

Test: Manual check - Subtitle view can show custom color subtitles from specific Subtitle
Renderer and outline is shown correctly using user defined color.
@nnoury nnoury force-pushed the fix/subtitles-outline-color branch from 259d487 to 1e72e1a Compare January 29, 2020 13:51
@nnoury
Copy link
Contributor Author

nnoury commented Jan 29, 2020

@icbaker @phhusson and I rewrote the background color logic to take into account various painter configurations when using outline : one apply the background only to the first pass painter when the painting is done in two passes

ojw28 added a commit that referenced this pull request Jan 30, 2020
@ojw28 ojw28 merged commit 1e72e1a into google:dev-v2 Jan 30, 2020
@google google locked and limited conversation to collaborators Mar 31, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants