Bug 157532 – Adjust "main content" video heuristic

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Eric Carlson]

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.

[Eric Carlson]

Created attachment 278734 [details]
Proposed patch

[Darin Adler]

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?

[Darin Adler]

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

[Darin Adler]

Besides the misspelling, none of my comments are mandatory changes; just ideas on how to do things a little better.

[Eric Carlson]

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.

[Eric Carlson]

Committed r200778: https://trac.webkit.org/r200778