Bug 81856 - HTMLMediaElement::updateActiveTextTrackCues can do unnecessary work
Summary: HTMLMediaElement::updateActiveTextTrackCues can do unnecessary work
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Eric Carlson
URL:
Keywords: InRadar
Depends on:
Blocks: 43668
  Show dependency treegraph
 
Reported: 2012-03-21 18:07 PDT by Eric Carlson
Modified: 2013-05-01 11:54 PDT (History)
8 users (show)

See Also:


Attachments
Proposed fix. (3.02 KB, patch)
2012-08-26 22:51 PDT, Edaena Salinas
eric.carlson: review-
Details | Formatted Diff | Diff
Proposed patch (1.52 KB, patch)
2013-05-01 11:25 PDT, Eric Carlson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Carlson 2012-03-21 18:07:42 PDT
HTMLMediaElement::updateActiveTextTrackCues is called several times a second to see if text track cues need to be rendered. Unfortunately, it does a fair amount of work even when there are no text tracks at all.
Comment 1 Edaena Salinas 2012-08-26 22:51:16 PDT
Created attachment 160640 [details]
Proposed fix.
Comment 2 Victor Carbune 2012-08-27 00:17:10 PDT
Comment on attachment 160640 [details]
Proposed fix.

View in context: https://bugs.webkit.org/attachment.cgi?id=160640&action=review

It's a good check, but I don't think it's enough to close this bug - it's about unnecessary work when there actually are tracks associated with the video.

> Source/WebCore/html/HTMLMediaElement.cpp:715
> +        if (RuntimeEnabledFeatures::webkitVideoTrackEnabled() && m_textTracks)

I think checking for m_textTracks should be in updateActiveTextTrackCues. Even better, HTMLMediaElement::ignoreTrackDisplayUpdateRequests() could become { return !m_textTracks || m_ignoreTrackDisplayUpdate > 0; }
Comment 3 Eric Carlson 2012-08-27 06:43:42 PDT
(In reply to comment #2)
> (From update of attachment 160640 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=160640&action=review
> 
> It's a good check, but I don't think it's enough to close this bug - it's about unnecessary work when there actually are tracks associated with the video.
> 
> > Source/WebCore/html/HTMLMediaElement.cpp:715
> > +        if (RuntimeEnabledFeatures::webkitVideoTrackEnabled() && m_textTracks)
> 
> I think checking for m_textTracks should be in updateActiveTextTrackCues. Even better, HTMLMediaElement::ignoreTrackDisplayUpdateRequests() could become { return !m_textTracks || m_ignoreTrackDisplayUpdate > 0; }

I agree with Victor. Additionally, the "LOG(...)" in HTMLMediaElement::updateActiveTextTrackCues should be after the ignoreTrackDisplayUpdateRequests check.
Comment 4 Radar WebKit Bug Importer 2013-05-01 11:24:41 PDT
<rdar://problem/13783424>
Comment 5 Eric Carlson 2013-05-01 11:25:53 PDT
Created attachment 200229 [details]
Proposed patch
Comment 6 WebKit Commit Bot 2013-05-01 11:54:39 PDT
Comment on attachment 200229 [details]
Proposed patch

Clearing flags on attachment: 200229

Committed r149443: <http://trac.webkit.org/changeset/149443>
Comment 7 WebKit Commit Bot 2013-05-01 11:54:41 PDT
All reviewed patches have been landed.  Closing bug.