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/
<rdar://problem/33121806>
Created attachment 377399 [details] A fix
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.
RenderImageControls is also a RenderImage derived class that would be effected by this change. Is that Okay?
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.
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.
Created attachment 377433 [details] An updated patch
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.
Created attachment 377540 [details] A new patch
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.
Created attachment 378581 [details] A updated patch after discussion with Daniel
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>.
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().
Created attachment 378896 [details] Patch for landing
Comment on attachment 378896 [details] Patch for landing r=me
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.
Created attachment 379070 [details] The patch for landing
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 ^^^.
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:832 > + Ok as-is. Ditto.
Comment on attachment 379070 [details] The patch for landing Clearing flags on attachment: 379070 Committed r250100: <https://trac.webkit.org/changeset/250100>
All reviewed patches have been landed. Closing bug.