Bug 139819 - [iOS] Log how often media element playback happens using FeatureCounter
Summary: [iOS] Log how often media element playback happens using FeatureCounter
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-12-19 10:44 PST by Chris Dumez
Modified: 2014-12-19 13:07 PST (History)
11 users (show)

See Also:


Attachments
Patch (3.21 KB, patch)
2014-12-19 10:48 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (6.23 KB, patch)
2014-12-19 11:46 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (5.23 KB, patch)
2014-12-19 12:10 PST, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2014-12-19 10:44:40 PST
Log how often media element playback happens using FeatureCounter.

Radar: <rdar://problem/19309988>
Comment 1 Chris Dumez 2014-12-19 10:48:34 PST
Created attachment 243555 [details]
Patch
Comment 2 Eric Carlson 2014-12-19 10:57:22 PST
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 3 Chris Dumez 2014-12-19 11:00:18 PST
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.).
Comment 4 Eric Carlson 2014-12-19 11:13:43 PST
(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.
Comment 5 Chris Dumez 2014-12-19 11:31:20 PST
(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.
Comment 6 Chris Dumez 2014-12-19 11:46:06 PST
Created attachment 243561 [details]
Patch
Comment 7 WebKit Commit Bot 2014-12-19 11:47:55 PST
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 8 Eric Carlson 2014-12-19 11:57:13 PST
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.
Comment 9 Chris Dumez 2014-12-19 11:58:53 PST
(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.
Comment 10 Chris Dumez 2014-12-19 12:10:25 PST
Created attachment 243562 [details]
Patch
Comment 11 WebKit Commit Bot 2014-12-19 12:11:29 PST
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 12 WebKit Commit Bot 2014-12-19 13:07:41 PST
Comment on attachment 243562 [details]
Patch

Clearing flags on attachment: 243562

Committed r177591: <http://trac.webkit.org/changeset/177591>
Comment 13 WebKit Commit Bot 2014-12-19 13:07:47 PST
All reviewed patches have been landed.  Closing bug.