Log how often media element playback happens using FeatureCounter. Radar: <rdar://problem/19309988>
Created attachment 243555 [details] Patch
Comment on attachment 243555 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=243555&action=review > Source/WebCore/html/HTMLMediaElement.cpp:4544 > + if (!currentMediaTime()) { > + // Log that a media element started playback. > + FEATURE_COUNTER_INCREMENT_KEY(document().page(), isVideo() ? FeatureCounterMediaPlayingVideoKey : FeatureCounterMediaPlayingAudioKey); > + } > + This will increment the counter every time a file loops or is rewound, so I think it is probably worth adding an instance variable to track the first time playback begins.
Comment on attachment 243555 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=243555&action=review >> Source/WebCore/html/HTMLMediaElement.cpp:4544 >> + > > This will increment the counter every time a file loops or is rewound, so I think it is probably worth adding an instance variable to track the first time playback begins. I thought about this case and thought it would be useful to log the case when we play the media again. In my opinion, playing a new media or playing the same media again is similar (in terms of power usage for e.g.).
(In reply to comment #3) > Comment on attachment 243555 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=243555&action=review > > >> Source/WebCore/html/HTMLMediaElement.cpp:4544 > >> + > > > > This will increment the counter every time a file loops or is rewound, so I think it is probably worth adding an instance variable to track the first time playback begins. > > I thought about this case and thought it would be useful to log the case > when we play the media again. In my opinion, playing a new media or playing > the same media again is similar (in terms of power usage for e.g.). OK. Probably worth adding a comment to that effect to avoid confusion.
(In reply to comment #4) > (In reply to comment #3) > > Comment on attachment 243555 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=243555&action=review > > > > >> Source/WebCore/html/HTMLMediaElement.cpp:4544 > > >> + > > > > > > This will increment the counter every time a file loops or is rewound, so I think it is probably worth adding an instance variable to track the first time playback begins. > > > > I thought about this case and thought it would be useful to log the case > > when we play the media again. In my opinion, playing a new media or playing > > the same media again is similar (in terms of power usage for e.g.). > > OK. Probably worth adding a comment to that effect to avoid confusion. I have just discussed this with Gavin and came to the conclusion that counting each media only once is more useful. We're also interested in the total number of media items (to get a ratio of total vs played). I'll update the patch accordingly.
Created attachment 243561 [details] Patch
Attachment 243561 [details] did not pass style-queue: ERROR: Source/WebCore/html/HTMLMediaElement.h:830: Please declare enum bitfields as unsigned integral types. [runtime/enum_bitfields] [5] Total errors found: 1 in 6 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 243561 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=243561&action=review > Source/WebCore/ChangeLog:16 > + (WebCore::HTMLMediaElement::parseAttribute): prepareChangeLog gets confused by some of the #if's in HTMLMediaElement.cpp and says every change is in HTMLMediaElement::parseAttribute. Can you change this to "WebCore::HTMLMediaElement::updatePlayState"? > Source/WebCore/html/HTMLVideoElement.cpp:59 > + > + // Log that we encountered a video element. > + FEATURE_COUNTER_INCREMENT_KEY(document.page(), FeatureCounterMediaVideoElementKey); I am not sure this count exactly what you want, because lots (and lots and lots) of pages create one or more temporary video/audio elements on page load to detect what media formats the browser supports. This is frequently done by libraries included by pages that don't have and audio or video to play. If you want to count the number of elements that are created that might play, I think it might make more sense to increment the count in HTMLMediaElement::loadResource where the src is set to a non-empty url.
(In reply to comment #8) > Comment on attachment 243561 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=243561&action=review > > > Source/WebCore/ChangeLog:16 > > + (WebCore::HTMLMediaElement::parseAttribute): > > prepareChangeLog gets confused by some of the #if's in HTMLMediaElement.cpp > and says every change is in HTMLMediaElement::parseAttribute. Can you change > this to "WebCore::HTMLMediaElement::updatePlayState"? Sure. > > Source/WebCore/html/HTMLVideoElement.cpp:59 > > + > > + // Log that we encountered a video element. > > + FEATURE_COUNTER_INCREMENT_KEY(document.page(), FeatureCounterMediaVideoElementKey); > > I am not sure this count exactly what you want, because lots (and lots and > lots) of pages create one or more temporary video/audio elements on page > load to detect what media formats the browser supports. This is frequently > done by libraries included by pages that don't have and audio or video to > play. If you want to count the number of elements that are created that > might play, I think it might make more sense to increment the count in > HTMLMediaElement::loadResource where the src is set to a non-empty url. Interesting, I did not know this. Thanks for catching it, I definitely need to update this then.
Created attachment 243562 [details] Patch
Attachment 243562 [details] did not pass style-queue: ERROR: Source/WebCore/html/HTMLMediaElement.h:830: Please declare enum bitfields as unsigned integral types. [runtime/enum_bitfields] [5] Total errors found: 1 in 4 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 243562 [details] Patch Clearing flags on attachment: 243562 Committed r177591: <http://trac.webkit.org/changeset/177591>
All reviewed patches have been landed. Closing bug.