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

• Component: Media
• Status: RESOLVED FIXED

[Anna Cavender]

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

[Anna Cavender]

Created attachment 137468 [details]
Patch

[Anna Cavender]

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]

(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 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]

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]

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

[Eric Carlson]

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]

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]

(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]

(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]

Created attachment 143077 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 143077 [details]
Patch for landing

Clearing flags on attachment: 143077

Committed r117811: <http://trac.webkit.org/changeset/117811>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.