Bug 139819 – [iOS] Log how often media element playback happens using FeatureCounter

• Component: Media
• Status: RESOLVED FIXED

[Chris Dumez]

Log how often media element playback happens using FeatureCounter.

Radar: <rdar://problem/19309988>

[Chris Dumez]

Created attachment 243555 [details]
Patch

[Eric Carlson]

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.

[Chris Dumez]

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.).

[Eric Carlson]

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

[Chris Dumez]

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

[Chris Dumez]

Created attachment 243561 [details]
Patch

[WebKit Commit Bot]

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.

[Eric Carlson]

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.

[Chris Dumez]

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

[Chris Dumez]

Created attachment 243562 [details]
Patch

[WebKit Commit Bot]

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.

[WebKit Commit Bot]

Comment on attachment 243562 [details]
Patch

Clearing flags on attachment: 243562

Committed r177591: <http://trac.webkit.org/changeset/177591>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.