Bug 83321

Summary:

Text tracks should not render unless they have kind=captions or kind=subtitles

Product:

WebKit

Reporter:

Anna Cavender <annacc>

Component:

Media

Assignee:

Anna Cavender <annacc>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

eric.carlson, feature-media-reviews, silviapf, vcarbune, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

43668

Attachments:

Description	Flags
Patch	none
Remove any currently rendered cues if kind changes to a non-visible kind. + tests.	none
Patch for landing	none

Anna Cavender

Reported 2012-04-05 15:14:27 PDT

Videos that have a track element with kind=metadata should not render captions. For example, captions should not be visible here: http://www.annacavender.com/track/demo1/track-metadata.html

Attachments
Patch (1.52 KB, patch) 2012-04-16 20:06 PDT, Anna Cavender	no flags	Details Formatted Diff Diff
Remove any currently rendered cues if kind changes to a non-visible kind. + tests. (11.33 KB, patch) 2012-04-25 15:09 PDT, Anna Cavender	no flags	Details Formatted Diff Diff
Patch for landing (11.35 KB, patch) 2012-05-21 12:38 PDT, Anna Cavender	no flags	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Anna Cavender

Comment 1 2012-04-16 20:06:17 PDT

Created attachment 137468 [details] Patch

Anna Cavender

Comment 2 2012-04-16 20:12:41 PDT

I'm not entirely convinced that updateDisplay() is the proper place to check the text track kind. Another solution would be to maintain a separate list of renderable TextTrackCues in HTMLMediaElement. My gut tells me that the overhead of maintaining that list is not worth it and that currentlyActiveCues() will typically be small enough that sifting them right before rendering is probably not a big deal. I'm open to suggestions. As for testing, I'm reluctant to add more pixel tests. Any other ideas?

Victor Carbune

Comment 3 2012-04-17 00:38:46 PDT

(In reply to comment #2) > I'm not entirely convinced that updateDisplay() is the proper place to check the text track kind. Another solution would be to maintain a separate list of renderable TextTrackCues in HTMLMediaElement. My gut tells me that the overhead of maintaining that list is not worth it and that currentlyActiveCues() will typically be small enough that sifting them right before rendering is probably not a big deal. I'm open to suggestions. > > As for testing, I'm reluctant to add more pixel tests. Any other ideas? You can call textTrackDisplayElement(video, 'display') defined LayoutTest/media/media-controls.js and check that it throws an error (hence no cue display block is there).

Eric Carlson

Comment 4 2012-04-19 10:41:46 PDT

Comment on attachment 137468 [details] Patch Marking r- because this needs a test. I agree that we DO NOT want any more pixel tests, but I think Victor's suggestion should work well.

Anna Cavender

Comment 5 2012-04-25 15:09:11 PDT

Created attachment 138879 [details] Remove any currently rendered cues if kind changes to a non-visible kind. + tests. Thanks for the suggestions, Victor

Anna Cavender

Comment 6 2012-05-17 12:22:28 PDT

Ping! I think the next review on this one fell through the cracks.

Eric Carlson

Comment 7 2012-05-17 14:39:04 PDT

Comment on attachment 138879 [details] Remove any currently rendered cues if kind changes to a non-visible kind. + tests. View in context: https://bugs.webkit.org/attachment.cgi?id=138879&action=review > Source/WebCore/html/HTMLMediaElement.cpp:1288 > + ExceptionCode unusedException; > + track->setMode(TextTrack::HIDDEN, unusedException); ASSERT_NO_EXCEPTION would be preferable. > Source/WebCore/html/track/TextTrackCue.cpp:614 > + ExceptionCode unusedException; > + m_displayTree->remove(unusedException); Ditto.

Anna Cavender

Comment 8 2012-05-20 22:10:32 PDT

Thanks Eric! (In reply to comment #7) > (From update of attachment 138879 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=138879&action=review > > > Source/WebCore/html/HTMLMediaElement.cpp:1288 > > + ExceptionCode unusedException; > > + track->setMode(TextTrack::HIDDEN, unusedException); > > ASSERT_NO_EXCEPTION would be preferable. Good idea, that makes sense here. > > > Source/WebCore/html/track/TextTrackCue.cpp:614 > > + ExceptionCode unusedException; > > + m_displayTree->remove(unusedException); > > Ditto. In this case, I'd have to add an ASSERT_NO_EXCEPTION in Node.h. Is this worth doing?

Eric Carlson

Comment 9 2012-05-21 09:23:56 PDT

(In reply to comment #8) > > > > > > Source/WebCore/html/track/TextTrackCue.cpp:614 > > > + ExceptionCode unusedException; > > > + m_displayTree->remove(unusedException); > > > > Ditto. > > In this case, I'd have to add an ASSERT_NO_EXCEPTION in Node.h. Is this worth doing? > "m_displayTree->remove(ASSERT_NO_EXCEPTION)" won't work?

Anna Cavender

Comment 10 2012-05-21 10:09:12 PDT

(In reply to comment #9) > (In reply to comment #8) > > > > > > > > > Source/WebCore/html/track/TextTrackCue.cpp:614 > > > > + ExceptionCode unusedException; > > > > + m_displayTree->remove(unusedException); > > > > > > Ditto. > > > > In this case, I'd have to add an ASSERT_NO_EXCEPTION in Node.h. Is this worth doing? > > > "m_displayTree->remove(ASSERT_NO_EXCEPTION)" won't work? Oh, yes, I misunderstood your original suggestion. I'll go ahead and land with those two additions. Thanks!

Anna Cavender

Comment 11 2012-05-21 12:38:35 PDT

Created attachment 143077 [details] Patch for landing

WebKit Review Bot

Comment 12 2012-05-21 13:33:38 PDT

Comment on attachment 143077 [details] Patch for landing Clearing flags on attachment: 143077 Committed r117811: <http://trac.webkit.org/changeset/117811>

WebKit Review Bot

Comment 13 2012-05-21 13:33:44 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.