174122 – HTMLVideoElement with a broken poster image will take square dimension

RESOLVED FIXED Bug 174122

HTMLVideoElement with a broken poster image will take square dimension

https://bugs.webkit.org/show_bug.cgi?id=174122

Summary HTMLVideoElement with a broken poster image will take square dimension

Virgil Spruit

Reported 2017-07-04 02:58:12 PDT

If the image of a poster can't be found the video will take a square dimension. Right click > Show Controls will place it back to it's true dimensions. Tested on El Capitan 10.11.6 - Safari 9.1.2 (11601.7.7) Sierra 10.12.5 - Safari 10.1.1 (12603.2.4) example: http://www.klash.nl/labs/video-poster.html mirror: https://jsfiddle.net/y7v2wf6o/

Attachments
A fix (3.97 KB, patch) 2019-08-27 16:19 PDT, Peng Liu	no flags	Details Formatted Diff Diff
An updated patch (4.81 KB, patch) 2019-08-28 00:03 PDT, Peng Liu	no flags	Details Formatted Diff Diff
A new patch (8.02 KB, patch) 2019-08-28 19:54 PDT, Peng Liu	no flags	Details Formatted Diff Diff
A updated patch after discussion with Daniel (11.89 KB, patch) 2019-09-11 15:35 PDT, Peng Liu	dbates: review+	Details Formatted Diff Diff
Patch for landing (11.79 KB, patch) 2019-09-16 14:44 PDT, Peng Liu	dbates: review+	Details Formatted Diff Diff
The patch for landing (10.12 KB, patch) 2019-09-18 13:57 PDT, Peng Liu	no flags	Details Formatted Diff Diff
Show Obsolete (5) View All Add attachment proposed patch, testcase, etc.

Radar WebKit Bug Importer

Comment 1 2017-07-04 04:13:02 PDT

<rdar://problem/33121806>

Peng Liu

Comment 2 2019-08-27 16:19:27 PDT

Created attachment 377399 [details] A fix

Daniel Bates

Comment 3 2019-08-27 20:11:54 PDT

Comment on attachment 377399 [details] A fix View in context: https://bugs.webkit.org/attachment.cgi?id=377399&action=review This is a good attempt. This is not the solution I expected to see because: 1. I did not expect to see the use of renderName() or strcmp() in the solution because: a. renderName() is a human-readable name for the renderer whose primary purpose it to produce human-readable dumps of the render tree b. Because of (a) renderName() is an indirect and fragile way of testing for the type of renderer. 2. I did not expect the fix to affect all non-video RenderImage derived classes beca > LayoutTests/media/video-poster-not-found.html:1 > +<html> This is ok as-is. This file would benefit by adding <!DOCTYPE html> to the top of this file so that it: 1. Doesn’t inadvertently make use of quirks mode 2. Actually test two things because of (1): a quirk mode quirk and the fix for this bug. > LayoutTests/media/video-poster-not-found.html:11 > + // When only the width (no height) attribute of a video element is given, > + // the aspect ratio of the video element should not be 1, which is the default behavior of WebKit > + // to paint a broken image. For thought, this kinda of comment would also be great to put in the LayoutTests ChangeLog in my opinion. > LayoutTests/media/video-poster-not-found.html:13 > + testExpected("video.clientHeight", 100); Haven’t read the code...where does the 100 come from? > LayoutTests/media/video-poster-not-found.html:14 > + endTest(); I think this test is ok because I think the DOM load event for the document (<body>) is delayed until all ImageLoaders have loaded or errored out, but I would have expected the ChangeLog to confirm this or explain what other mechanism ensures that a missing poster image is guaranteed to fall before the DOM load event is fired. > LayoutTests/media/video-poster-not-found.html:21 > + <p>Test <video> element with only width attribute and the poster is invalid.</p> This is ok-as is. I think the more technically correct way to explain this is to use the word “missing” instead of “invalid” to describe what this test is doing because: 1. What is an “invalid” poster image requires a person to read the spec. That definition could turn out to include a missing image resource, but more likely “invalid” probably includes malformed data (e.g. poster image is an HTML document not an image), bad MIME type, unhandled MIME type, etc etc. 2. Seems like it better matches with the test <title> and filename. 3. At least to me, the word “missing” is less ambiguous because of the cases for (1). > Source/WebCore/ChangeLog:12 > +Â Â Â Â an invalid poster image will make its aspect ratio to be 1, which is the historical WebKit behavior > +Â Â Â Â if we're painting alt text and/or a broken image. > +Â Â Â Â This fix prevents that behavior to impact video elements. There seems to be some fancy characters used to indent these lines. Please remove them.

Daniel Bates

Comment 4 2019-08-27 20:15:36 PDT

RenderImageControls is also a RenderImage derived class that would be effected by this change. Is that Okay?

Peng Liu

Comment 5 2019-08-27 23:10:46 PDT

Thanks a lot for your comments! I will revise the patch accordingly. By the way, could you confirm that RenderImageControls is a RenderImage derived class? I did some search and found that only RenderMedia and RenderVideo are its derived classes.

Peng Liu

Comment 6 2019-08-27 23:58:17 PDT

Comment on attachment 377399 [details] A fix View in context: https://bugs.webkit.org/attachment.cgi?id=377399&action=review >> LayoutTests/media/video-poster-not-found.html:1 >> +<html> > > This is ok as-is. This file would benefit by adding <!DOCTYPE html> to the top of this file so that it: > > 1. Doesn’t inadvertently make use of quirks mode > 2. Actually test two things because of (1): a quirk mode quirk and the fix for this bug. Good point! >> LayoutTests/media/video-poster-not-found.html:11 >> + // to paint a broken image. > > For thought, this kinda of comment would also be great to put in the LayoutTests ChangeLog in my opinion. Right. I will change the ChangeLog. >> LayoutTests/media/video-poster-not-found.html:13 >> + testExpected("video.clientHeight", 100); > > Haven’t read the code...where does the 100 come from? It is from the default behavior of a video element when its source is not given. The default aspect ratio is 2, so the height will be 200 / 2 = 100. I will modify the test case to use a video source, so that the height will be decided by the content. >> LayoutTests/media/video-poster-not-found.html:14 >> + endTest(); > > I think this test is ok because I think the DOM load event for the document (<body>) is delayed until all ImageLoaders have loaded or errored out, but I would have expected the ChangeLog to confirm this or explain what other mechanism ensures that a missing poster image is guaranteed to fall before the DOM load event is fired. I will modify the test case by providing a source to the video element. Then the size of the video element is verify in the handler for "canplaythrough" event. I think that can guarantee the failure regarding the missing poster image will happen before we check the element size. >> LayoutTests/media/video-poster-not-found.html:21 >> + <p>Test <video> element with only width attribute and the poster is invalid.</p> > > This is ok-as is. I think the more technically correct way to explain this is to use the word “missing” instead of “invalid” to describe what this test is doing because: > > 1. What is an “invalid” poster image requires a person to read the spec. That definition could turn out to include a missing image resource, but more likely “invalid” probably includes malformed data (e.g. poster image is an HTML document not an image), bad MIME type, unhandled MIME type, etc etc. > 2. Seems like it better matches with the test <title> and filename. > 3. At least to me, the word “missing” is less ambiguous because of the cases for (1). Right. >> Source/WebCore/ChangeLog:12 >> +Â Â Â Â This fix prevents that behavior to impact video elements. > > There seems to be some fancy characters used to indent these lines. Please remove them. They were introduced by the editor. I will remove them. > Source/WebCore/rendering/RenderImage.cpp:824 > + if (imageResource().errorOccurred() && !strcmp(renderName(), "RenderImage")) { I will replace the strcmp() with isImage(). RenderMedia overrides isImage() function and it returns false. RenderVideo inherits that behavior. Other derived class of RenderImage can choose to inherit RenderImage's behavior to avoid the potential issue of this fix.

Peng Liu

Comment 7 2019-08-28 00:03:14 PDT

Created attachment 377433 [details] An updated patch

Daniel Bates

Comment 8 2019-08-28 10:23:20 PDT

Comment on attachment 377433 [details] An updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=377433&action=review > Source/WebCore/rendering/RenderImage.cpp:824 > + if (imageResource().errorOccurred() && isImage()) { This looks better. Even with this change I (just me) still would not expect to see a isImage() check in the solution or I would expect to see an explanation in the ChangeLog why this check is correct and why it is correct to use in this function. The reasons why I feel this check is unexpected is because: 1. This bug is about video. So, I expected a solution-specific to just <video> or an explanation in the ChangeLog why the fix is applicable to other non-video image things. 2. I would have expected isImage() to be used from non-derived renderer classes. Here's another way of expressing this: RenderImage is an image so to an to unfamiliar developer isImage() would be expected to vacuously return true - it doesn't. 3. RenderImage is a **base** class for all current and future Image renderers, including <img>, <audio>, <video>, generated images, etc etc.... this gets me back to point (1). 4. RenderImage::computeIntrinsicRatioInformation() is a virtual function and RenderMedia is a derived class. Falling out from (1) and (3) I would have expected a derived class to override this function as a means of changing behavior as opposed to the base class "looking into itself" <-- not sure if I am conveying this correctly, trying to convey the weirdness of point (2) 5. By (1) and (4) I would have expected the solution to be overriding computeIntrinsicRatioInformation() in RenderVideo or an explanation in the ChangeLog why this is not the correct solution.

Peng Liu

Comment 9 2019-08-28 19:54:42 PDT

Created attachment 377540 [details] A new patch

Daniel Bates

Comment 10 2019-08-29 09:27:34 PDT

The patch looks good. I was expecting the virtual function to also be used when deciding to paint the broken image icon from our in person conversation, yesterday, and possibly any other applicable code we missed yesterday.

Peng Liu

Comment 11 2019-09-11 15:35:25 PDT

Created attachment 378581 [details] A updated patch after discussion with Daniel

Daniel Bates

Comment 12 2019-09-13 15:33:23 PDT

Comment on attachment 378581 [details] A updated patch after discussion with Daniel View in context: https://bugs.webkit.org/attachment.cgi?id=378581&action=review Your change log message is very descriptive. I like it. > Source/WebCore/rendering/RenderImage.cpp:463 > + RefPtr<Image> image = brokenImageAndImageScaleFactor.first; It would be slightly more optimal to use auto& here instead of RefPtr<> (you'll need to * the right-hand side) because brokenImageAndImageScaleFactor.first is the broken image, which is guaranteed never be nullptr and exist for the entire program lifetime. So, no need to ref it. Optional: This doesn't need to be done in this patch callers of CachedImage::brokenImage() would benefit if it returned a std::pair<std::reference_wrapper<const Image>, float> instead of std::pair<Image*, float>. If std::pair<std::reference_wrapper<const Image>, float> doesn't work, then std::pair<std::reference_wrapper<Image>, float>.

Daniel Bates

Comment 13 2019-09-13 16:01:52 PDT

Comment on attachment 378581 [details] A updated patch after discussion with Daniel View in context: https://bugs.webkit.org/attachment.cgi?id=378581&action=review > Source/WebCore/rendering/RenderImage.cpp:460 > - if (imageResource().errorOccurred() && !image->isNull() && usableSize.width() >= image->width() && usableSize.height() >= image->height()) { > + if (shouldDisplayBrokenImageIcon()) { This introduces another behavioral change that seems unrelated to this bug. It seems like it could be a good change. Making the change now introduces risk. It may be more beneficial to factor out this change and the change in lines 467-479 into another patch and just replace the first conjunct (or first and second conjunct - can't remember what we talked about) with shouldDisplayBrokenImageIcon().

Peng Liu

Comment 14 2019-09-16 14:44:17 PDT

Created attachment 378896 [details] Patch for landing

Daniel Bates

Comment 15 2019-09-18 13:15:33 PDT

Comment on attachment 378896 [details] Patch for landing r=me

Daniel Bates

Comment 16 2019-09-18 13:17:30 PDT

Comment on attachment 378896 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=378896&action=review > Source/WebCore/rendering/RenderImage.cpp:482 > + // Make sure we have enough space to display the broken image icon > + if (usableSize.width() >= imageSize.width() && usableSize.height() >= imageSize.height()) { > + // Center the error image, accounting for border and padding. > + LayoutUnit centerX { (usableSize.width() - imageSize.width()) / 2 }; > + if (centerX < 0) > + centerX = 0; > + LayoutUnit centerY { (usableSize.height() - imageSize.height()) / 2 }; > + if (centerY < 0) > + centerY = 0; > + imageOffset = LayoutSize(leftBorder + leftPad + centerX + missingImageBorderWidth, topBorder + topPad + centerY + missingImageBorderWidth); > + > + context.drawImage(*image, snapRectToDevicePixels(LayoutRect(paintOffset + imageOffset, imageSize), deviceScaleFactor), { imageOrientation() }); > + errorPictureDrawn = true; > + } This introduces another behavioral change that seems unrelated to this bug. It seems like it could be a good change. Making the change now introduces risk. It may be more beneficial to factor out this change into another patch.

Peng Liu

Comment 17 2019-09-18 13:57:21 PDT

Created attachment 379070 [details] The patch for landing

Daniel Bates

Comment 18 2019-09-19 10:08:16 PDT

Comment on attachment 379070 [details] The patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=379070&action=review > Source/WebCore/rendering/RenderImage.cpp:468 > + OK as-is. Added a empty line here ^^^.

Daniel Bates

Comment 19 2019-09-19 10:08:31 PDT

WebKit Commit Bot

Comment 20 2019-09-19 12:19:43 PDT

Comment on attachment 379070 [details] The patch for landing Clearing flags on attachment: 379070 Committed r250100: <https://trac.webkit.org/changeset/250100>

WebKit Commit Bot

Comment 21 2019-09-19 12:19:45 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version Safari 10

Hardware Mac

OS All

Product WebKit

Component Media

Assignee

Peng Liu

Reported

2017-07-04 02:58 PDT

Modified

2019-09-19 12:19 PDT History

CC List

13 users Show

URL

Keywords InRadar

Depends on

Blocks