Bug 157532 - Adjust "main content" video heuristic
Summary: Adjust "main content" video heuristic
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Eric Carlson
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-05-10 13:31 PDT by Eric Carlson
Modified: 2016-05-12 10:58 PDT (History)
2 users (show)

See Also:


Attachments
Proposed patch (9.78 KB, patch)
2016-05-12 08:36 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 2016-05-10 13:31:51 PDT
Videos smaller than 400x300 are never considered "main content". Adjust the heuristic so it doesn't depend on the area and aspect ratio rather than the absolute width and height.
Comment 1 Eric Carlson 2016-05-12 08:36:21 PDT
Created attachment 278734 [details]
Proposed patch
Comment 2 Darin Adler 2016-05-12 08:48:14 PDT
Comment on attachment 278734 [details]
Proposed patch

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

> Source/WebCore/html/MediaElementSession.cpp:555
> +    RenderBox* renderer = downcast<RenderBox>(element.renderer());

This cast to RenderBox is not good. Since HTMLMediaElement guarantees the renderer is RenderMedia, that should be expressed by HTMLMediaElement itself. This would be done by overriding renderer in HTMLMediaElement to return a RenderMedia*, following the same pattern that ContainerNode uses to override renderer to return RenderElement. The inline function definition of HTMLMediaElement::renderer would go into RenderMedia.h, by analogy with how the ContainerNode::renderer inline function definition is in RenderElement.h.

The local variable here would then be:

    auto* renderer = element.renderer();

And the same in all the other places that call renderer on media elements.

> Source/WebCore/html/MediaElementSession.cpp:571
> +    if (width * height < elementMainContentAreaMinimum)
> +        return false;
> +
> +    double aspectRatio = width / height;
> +    if (aspectRatio < mimimumAspectRatio)
> +        return false;
> +
> +    if (aspectRatio > maximiumAspectRatio)
> +        return false;
> +
> +    return true;

I like the idea of writing it like this instead:

    double area = width * height;
    double aspectRatio = width / height;
    return area >= elementMainContentAreaMinimum && aspectRatio >= minimumAspectRatio && aspectRatio <= maximumAspectRatio;

What do you think?
Comment 3 Darin Adler 2016-05-12 08:48:32 PDT
Comment on attachment 278734 [details]
Proposed patch

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

> Source/WebCore/html/MediaElementSession.cpp:551
> +    static const double maximiumAspectRatio = 1.8; // Slightly larger than 16:9.

maximum is misspelled here
Comment 4 Darin Adler 2016-05-12 08:49:27 PDT
Besides the misspelling, none of my comments are mandatory changes; just ideas on how to do things a little better.
Comment 5 Eric Carlson 2016-05-12 10:56:01 PDT
Thanks for the suggestions! I fixed the spellings and changed the area and aspect ratio test as per your suggestions. The "renderer()" change requires modifications to several other files, so I will make those changes in a follow-up patch.
Comment 6 Eric Carlson 2016-05-12 10:58:32 PDT
Committed r200778: https://trac.webkit.org/r200778