449156 - Implement the poster attribute for the <video> element

Status: RESOLVED FIXED

(Core :: Audio/Video, defect, P2)

Description

[cajbir (:cajbir)]

From the spec in the URL:

"The poster  attribute gives the address of an image file that the user agent can show while no video data is available. The attribute, if present, must contain a valid URL. If the specified resource is to be used, it must be fetched when the element is created or when the poster attribute is set. The poster frame is then the image obtained from that resource, if any."

Comment 1

[Asa Dotzler [:asa]]

Without this feature, the initial experience of encountering a <video> is pretty bad. It's a big black box with no obvious purpose and doesn't even show controls until you mouse over (assuming you're using the controls attr.) 

Alternatively, we could make the controls show up (maybe with a big "play" button in the center of the <vide> element's area) until they'd been used and then auto-hide. 

Both might be desirable but the current state is a poor user experience -- especially for people new to the technology.

Comment 2

[cajbir (:cajbir)]

<video> displays the first frame, not a black box. So if the video is arranged to have a visible first frame that becomes the poster. For example: http://tinyvid.tv/show/35jcvnhftsrpg

This, and the fact that poster can be emulated with JavaScript, was the main reason we'd decided to hold off on this bug till 3.2.

I agree it's not a great user experience in the case where the first frame isn't good, or scripting is disabled. The latter is a problem due to bug 449358 anyway.

Bug 469211 covers the issue of controls needing to be moused over.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The usability issues here would be fixed by bug 469211 IMHO. We definitely won't be investing in 'poster' for 1.9.1.

Comment 4

[Biju]

(In reply to comment #1)
> Alternatively, we could make the controls show up (maybe with a big "play"
> button in the center of the <vide> element's area) until they'd been used and
> then auto-hide. 

created bug 488003

Comment 5

[Chris Pearce [:cpearce (Not reading bugmail)]]

Note to self: I talked to roc about this today. An easy way to implement this is to add an anonymous image element to nsVideoFrame which we display when appropriate. We need to ensure that we handle dynamic changes to poster as well.

Comment 6

[Wladimir Palant]

Note that this attribute should ideally change the buffering behavior as well - if it is present there is no need to request even the first frame of the video from the server (unless there is an autobuffer attribute).

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We have to decode the first frame just to get its start time, which we need for the video metadata like 'duration', so I don't think we can change the buffering behaviour.

Comment 8

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v1

Implement HTMLVideoElement.poster by adding an image element as an anonymous child to nsVideoFrame. Includes several reftests. Mochitests and reftests pass on Windows and Linux.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+void nsHTMLMediaElement::HidePosterFrame()
+{
+  // Set the flag which causes the video poster frame to hide.
+  mHasPlayedOrSeeked = PR_TRUE;

We should call the method HasPlayedOrSeeked, I think.

We should reset mHasPlayedOrSeeked somewhere when there's a new load, so it can display a poster, right? We'll want a test for that.

+  // Force a reflow so that the poster frame hides right now.

We should do this only if mHasPlayedOrSeeked was false, right?

+  presShell->FrameNeedsReflow(frame,
+                              nsIPresShell::eStyleChange,
+                              NS_FRAME_IS_DIRTY);

Can't this just be a reflow, not a style change?

+    if (child->GetType() == nsGkAtoms::imageFrame && ShouldDisplayPoster()) {

I think we should reflow the image frame even if the poster is not being displayed.

+  if (!HasVideoData()) {
+    // Not <video>, won't have poster.

Not really true. It could be <video> but the resource has no video data.

+nsVideoFrame::UpdatePosterSource()
+{
+  if (!HasVideoData()) {
+    // Only <video> tags have poster attributes.
+    return NS_OK;

So this is wrong, I think. If we have <video>, a poster, but no video data, we should display the poster. You can QI to nsIDOMHTMLVideoElement to check what kind of element we've got. You'll want to add a test for this.

+    nsPresContext* presContext = PresContext();
+    nsNodeInfoManager *nodeInfoManager =
+      presContext->Document()->NodeInfoManager();

Probably easier to just do element->GetCurrentDoc().

It looks to me like if the poster image is animated, we'll keep invalidating even when it's not being displayed. It would be good to give the poster image visibility:hidden while it's not being displayed. A simple way to do that would be to set its class and use an html.css style rule video > img.hidden { visibility:hidden; }. Then you could simplify some nsVideoFrame code a little bit.

It looks like there's a bug where we create the video frame while there is no poster attribute, and then the attribute gets set. You set mPosterImage, but it doesn't get added as the anonymous content for the nsVideoFrame because CreateAnonymousContent only gets called at frame construction. You either need to force a reframe of the video element, or always create an image for mPosterImage in CreateAnonymousContent. You'll want to add a test for that.

Comment 10

[Jeff Walden [:Waldo]]

Comment on attachment 382917 [details] [diff] [review]
Patch v1

>   PRPackedBool mLoadingEnabled : 1;
>   PRPackedBool mStartingLoad : 1;
> 
>   /**
>+   * When true, we return mForcedImageState from ImageState().
>+   */
>+  PRPackedBool mIsImageStateForced;
>+
>+  /**
>    * The state we had the last time we checked whether we needed to notify the
>    * document of a state change.  These are maintained by UpdateImageState.
>    */
>   PRPackedBool mLoading : 1;
>   PRPackedBool mBroken : 1;
>   PRPackedBool mUserDisabled : 1;
>   PRPackedBool mSuppressed : 1;

One of these things is not like the other things...

http://www.youtube.com/watch?v=tZIvgQ9ik48

Comment 11

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to comment #9)
> +void nsHTMLMediaElement::HidePosterFrame()
> +{
> +  // Set the flag which causes the video poster frame to hide.
> +  mHasPlayedOrSeeked = PR_TRUE;
> 
> We should call the method HasPlayedOrSeeked, I think.

We already have PRBool HasPlayedOrSeeked() which gets mHasPlayedOrSeeked, so how about:

void SetPlayedOrSeeked(PRBool)
PRBool GetPlayedOrSeeked()

We can use SetPlayedOrSeeked(PR_FALSE) to reset when we do a new load.

> +  presShell->FrameNeedsReflow(frame,
> +                              nsIPresShell::eStyleChange,
> +                              NS_FRAME_IS_DIRTY);
> 
> Can't this just be a reflow, not a style change?

Ok, I'm new to reflow, how do I do that?

> +  if (!HasVideoData()) {
> +    // Not <video>, won't have poster.
> 
> Not really true. It could be <video> but the resource has no video data.
> 
> +nsVideoFrame::UpdatePosterSource()
> +{
> +  if (!HasVideoData()) {
> +    // Only <video> tags have poster attributes.
> +    return NS_OK;
> 
> So this is wrong, I think. If we have <video>, a poster, but no video data, we
> should display the poster. You can QI to nsIDOMHTMLVideoElement to check what
> kind of element we've got. You'll want to add a test for this.

Ok, nsVideoFrame::HasVideoData() only QIs to nsIDOMHTMLVideoElement, so I'll add appropriate checks to the videoWidth and videoHeight as we discussed.

Thanks!

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #11)
> We already have PRBool HasPlayedOrSeeked() which gets mHasPlayedOrSeeked, so
> how about:
> 
> void SetPlayedOrSeeked(PRBool)
> PRBool GetPlayedOrSeeked()
> 
> We can use SetPlayedOrSeeked(PR_FALSE) to reset when we do a new load.

Sounds good.

> > +  presShell->FrameNeedsReflow(frame,
> > +                              nsIPresShell::eStyleChange,
> > +                              NS_FRAME_IS_DIRTY);
> > 
> > Can't this just be a reflow, not a style change?
> 
> Ok, I'm new to reflow, how do I do that?

eTreeChange

Comment 13

[Chris Pearce [:cpearce (Not reading bugmail)]]

For future reference - discussion on poster layout/sizing: http://lists.whatwg.org/pipermail/whatwg-whatwg.org/2008-June/015042.html

Comment 14

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v2

Patch v2. Addresses review comments, includes more tests. Video reftests and mochitests pass on Windows and Linux.

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+void nsHTMLMediaElement::HidePosterFrame()
+{

I still don't really like the name. I thought we were going to make this SetPlayedOrSeeked and do the hide-poster-frame stuff when the parameter is PR_TRUE.

What's the point of tracking the load ID in nsVideoFrame? Why can't you just call GetHasPlayedOrSeeked?

Comment 16

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to comment #15)
> +void nsHTMLMediaElement::HidePosterFrame()
> +{
> 
> I still don't really like the name. I thought we were going to make this
> SetPlayedOrSeeked and do the hide-poster-frame stuff when the parameter is
> PR_TRUE.
> 

Sorry, I interpreted your earlier comment to mean you wanted an accessor/modifer for mHasPlayedOrSeeked, rather than renaming HidePosterFrame().

> What's the point of tracking the load ID in nsVideoFrame? Why can't you just
> call GetHasPlayedOrSeeked?

It prevents us displaying the poster frame after we've displayed a frame for that video. We need to track the load IDs in case the video element is load()ed with a new resource with a poster - we don't want to not show the poster because the previously loaded video displayed a video frame. And if we add a poster after we've displayed a video frame, but not played or seeked, we don't want to display the poster frame. I have just made a testcase that breaks without this, I'll add it to the next iteration of the patch.

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

> And if we add a poster after we've displayed a video frame, but not played or
> seeked, we don't want to display the poster frame.

Why not? We should be displaying whatever poster there is whenever the video has not played or seeked, right? It shouldn't matter when the attribute was set.

Comment 18

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to comment #17)
> > And if we add a poster after we've displayed a video frame, but not played or
> > seeked, we don't want to display the poster frame.
> 
> Why not? We should be displaying whatever poster there is whenever the video
> has not played or seeked, right? It shouldn't matter when the attribute was
> set.

The spec says: "the poster frame should not be shown again after a frame of video has been shown."

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

That sounds wrong. Generally we want a DOM to be rendered the same no matter how it got into that state. I think we should stick with the GetPlayedOrSeeked test and request a change (or clarification) in the spec.

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

An example of where this could matter is if someone writes code that starts a video load, then later sets the poster attribute. If during testing the video is usually slow to load so the attribute is set before the first frame is loaded, the developer will see the poster and assume everything's fine. But if something (e.g. network conditions) changes so that the first frame loads before the attribute is set, it would be bad if the code to set the poster suddenly stops working.

Comment 21

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v3

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+{
+  nsHTMLVideoElement* element = static_cast<nsHTMLVideoElement*>(GetContent());
+  if (!HasVideoElement() ||
+      !element ||
+      (element->GetPlayedOrSeeked() && HasVideoData()))
+    return PR_FALSE;

element can't be null here. It would be better to return false if !HasVideoElement() before you cast the pointer to something it's not.

+  if (!element)
+    return NS_ERROR_FAILURE;

element can't be null here either. GetContent() can never return null, basically.

+  nsresult res = mPosterImage->SetAttr(kNameSpaceID_None,
+                                       nsGkAtoms::src,
+                                       posterStr,
+                                       PR_TRUE);

You can't do a notifying SetAttr during frame construction (which includes CreateAnonymousContent). So pass in an aNotify parameter and pass false from CreateAnonymousContent.

+    // If we have or have hidden a poster frame, or we may have changed size,
+    // so reflow.
+    PresContext()->PresShell()->FrameNeedsReflow(this,
+                                                 nsIPresShell::eTreeChange,
+                                                 NS_FRAME_IS_DIRTY);

Do we actually need this? The image frame should fire a notification itself when it loads.

+    // Flush reflow so that if we've hidden the poster, the video frame is
+    // displayed immediately.
+    PresContext()->PresShell()->FlushPendingNotifications(Flush_Frames);

Why do you need to do this? This shouldn't be needed, notifications will eventually be flushed.

+
+/* Video poster frames should scale to the size of their computed style box. */
+video > img {
+  width: 100%;
+  height: 100%;
+}

I don't think we need this, given your changes to Reflow().

Comment 23

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to comment #22)
> +    // If we have or have hidden a poster frame, or we may have changed size,
> +    // so reflow.
> +    PresContext()->PresShell()->FrameNeedsReflow(this,
> +                                                 nsIPresShell::eTreeChange,
> +                                                 NS_FRAME_IS_DIRTY);
> 
> Do we actually need this? The image frame should fire a notification itself
> when it loads.

When I don't do this sometimes when we change the poster attribute to an image of a different size, the new poster is rendered at the old size. After testing it further I realised that the reflow & flush doesn't prevent this all the time, so it's wrong anyway.

The problem here is that because we're forcing the image state to 0, the poster's anonymous image element's nsImageLoadingContent doesn't detect the image state change when it loads, and so doesn't reframe when it's finished decoding, so no reflow happens. I can fix this by scheduling a reflow in nsImageLoadingContent::OnStopDecode() when we're forcing the image state.

Comment 24

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v4

With review comments addressed. Mochitests and reftest pass on Windows and Linux.

Comment 25

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  doc->FlushPendingNotifications(Flush_Frames);

Why do we need this in SetPlayedOrSeeked?

+      nsSize availableSize = ShouldDisplayPoster()
+                           ? nsSize(aReflowState.availableWidth,
+                                    aReflowState.availableHeight)
+                           : nsSize(0,0);

I should have spotted this before. You don't want to set the available size to 0,0 if !ShouldDisplayPoster, instead you want to SetComputedWidth/SetComputedHeight to 0,0.

+  // If we're forcing image state, UpdateImageState() won't detect when the 
+  // image finishes loading, and won't reframe the image. We need to reflow
+  // the image frame, otherwise changes in size etc won't show up.
+  if (mIsImageStateForced) {
+    nsIDocument* doc = GetOurDocument();
+    if (!doc) return NS_ERROR_FAILURE;
+    nsIPresShell *presShell = doc->GetPrimaryShell();  
+    if (!presShell) return NS_ERROR_FAILURE;
+    nsCOMPtr<nsIContent> thisContent = do_QueryInterface(this);
+    if (!thisContent) return NS_ERROR_FAILURE;
+    nsIFrame* frame = presShell->GetPrimaryFrameFor(thisContent);
+    if (!frame) return NS_ERROR_FAILURE;
+    presShell->FrameNeedsReflow(frame,
+                                nsIPresShell::eTreeChange,
+                                NS_FRAME_IS_DIRTY);
+  }

I don't think doing this in OnStopDecode is actually right. We should reflow when the size of the image becomes known, which is generally near the start, not the end of loading. And I don't think we should do this in the content. I think we actually don't understand exactly why this is needed, yet. The image frame should normally change its intrinsic size in OnStartContainer/OnStopDecode if necessary, and forcing the image state shouldn't change those.

Comment 26

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to comment #25)
> +  doc->FlushPendingNotifications(Flush_Frames);
> 
> Why do we need this in SetPlayedOrSeeked?

Looks like it's unnecessary now.

 
> +  // If we're forcing image state, UpdateImageState() won't detect when the 
> +  // image finishes loading, and won't reframe the image. We need to reflow
> +  // the image frame, otherwise changes in size etc won't show up.
> +  if (mIsImageStateForced) {
> +    nsIDocument* doc = GetOurDocument();
> +    if (!doc) return NS_ERROR_FAILURE;
> +    nsIPresShell *presShell = doc->GetPrimaryShell();  
> +    if (!presShell) return NS_ERROR_FAILURE;
> +    nsCOMPtr<nsIContent> thisContent = do_QueryInterface(this);
> +    if (!thisContent) return NS_ERROR_FAILURE;
> +    nsIFrame* frame = presShell->GetPrimaryFrameFor(thisContent);
> +    if (!frame) return NS_ERROR_FAILURE;
> +    presShell->FrameNeedsReflow(frame,
> +                                nsIPresShell::eTreeChange,
> +                                NS_FRAME_IS_DIRTY);
> +  }
> 
> I don't think doing this in OnStopDecode is actually right. We should reflow
> when the size of the image becomes known, which is generally near the start,
> not the end of loading. And I don't think we should do this in the content. I
> think we actually don't understand exactly why this is needed, yet. The image
> frame should normally change its intrinsic size in
> OnStartContainer/OnStopDecode if necessary, and forcing the image state
> shouldn't change those.

Wow. It turns out this isn't necessary either. The problem this was fixing was caused by the "video > img {width: 100%;height: 100%;}" rule I added to ua.css in patch v3. Now that that style rule is gone, we don't need this extra reflow, and everything works as expected! Patch v5 to follow.

Comment 27

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v5

Comment 28

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 384059 [details] [diff] [review]
Patch v5

Need jst's review on the nsImageLoadingContent change.

Comment 29

[Johnny Stenback (:jst)]

Comment on attachment 384059 [details] [diff] [review]
Patch v5

r=jst for the nsImageLoadingContent changes

Comment 30

[Chris Pearce [:cpearce (Not reading bugmail)]]

Pushed: http://hg.mozilla.org/mozilla-central/rev/eb8abba2dd8f

Comment 31

[Chris Pearce [:cpearce (Not reading bugmail)]]

Nuts! Test failures in reftest test-poster9.html on OSX:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1245990428.1245995184.1637.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1245994272.1245996317.13939.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1245991902.1245993528.16418.gz

Backed out:
http://hg.mozilla.org/mozilla-central/rev/311d64a1a537 (push)
http://hg.mozilla.org/mozilla-central/rev/cd18c14bf7d2 (merge)

Comment 32

[Chris Pearce [:cpearce (Not reading bugmail)]]

Pushed fix: http://hg.mozilla.org/mozilla-central/rev/16049cb4fe43

The reftest test-poster9.html referred to a file which moved when bug 499880 landed at the same time.

Comment 33

[Chris Pearce [:cpearce (Not reading bugmail)]]

Removed test-poster9.html: http://hg.mozilla.org/mozilla-central/rev/65eda8445668

It seems to failed on tinderbox, probably because the tinderbox machines don't have sound.

Comment 34

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to comment #33)
> Removed test-poster9.html:
> http://hg.mozilla.org/mozilla-central/rev/65eda8445668
> 
> It seems to failed on tinderbox, probably because the tinderbox machines don't
> have sound.

Filed bug 500628 to track & fix this.

Comment 35

[Chris Pearce [:cpearce (Not reading bugmail)]]

Push: http://hg.mozilla.org/mozilla-central/rev/3c9461d415ee
Didn't remove the test properly the first time. :(

Comment 36

[John Drinkwater (:beta)]

Is there a likelihood this could end up in 3.5.1? Seems quite a useful part of the  video spec.

Comment 37

[Jeff Walden [:Waldo]]

I would presume no, since you can implement your own poster functionality in JavaScript without too much trouble, also since 3.5 is now a stable release that's only going to accept security and stability fixes.  I can imagine scenarios in which it might be accepted, but I don't think it's very likely to happen.

Comment 38

[Mike Beltzner [:beltzner, not reading bugmail]]

Mass change: adding fixed1.9.2 keyword

(This bug was identified as a mozilla1.9.2 blocker which was fixed before the mozilla-1.9.2 repository was branched (August 13th, 2009) as per this query: http://is.gd/2ydcb - if this bug is not actually fixed on mozilla1.9.2, please remove the keyword. Apologies for the bugspam)

Comment 39

[Eric Shepherd [:sheppy]]

Now documented here:

https://developer.mozilla.org/En/HTML/Element/Video