Bug 81856

Summary: HTMLMediaElement::updateActiveTextTrackCues can do unnecessary work
Product: WebKit Reporter: Eric Carlson <eric.carlson>
Component: MediaAssignee: Eric Carlson <eric.carlson>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, esprehn+autocc, feature-media-reviews, glenn, jer.noble, vcarbune, webkit-bug-importer, webkit.review.bot
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 43668    
Attachments:
Description Flags
Proposed fix.
eric.carlson: review-
Proposed patch none

Eric Carlson
Reported 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.
Attachments
Proposed fix. (3.02 KB, patch)
2012-08-26 22:51 PDT, Edaena Salinas 		eric.carlson: review-
DetailsFormatted DiffDiff
Proposed patch (1.52 KB, patch)
2013-05-01 11:25 PDT, Eric Carlson 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Edaena Salinas
Comment 1 2012-08-26 22:51:16 PDT
Created attachment 160640 [details] Proposed fix.
Victor Carbune
Comment 2 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; }
Eric Carlson
Comment 3 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.
Radar WebKit Bug Importer
Comment 4 2013-05-01 11:24:41 PDT
<rdar://problem/13783424>
Eric Carlson
Comment 5 2013-05-01 11:25:53 PDT
Created attachment 200229 [details] Proposed patch
WebKit Commit Bot
Comment 6 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>
WebKit Commit Bot
Comment 7 2013-05-01 11:54:41 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.