448674 - Allow <video> to be used as element source in drawImage

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect, P3)

Description

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

As the summary says -- <video> should be a valid source for drawImage; it should render whatever the current frame is.

Comment 1

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: Patch, v0

Fairly straightforward patch -- a little gritty due to rework/indentation changes in the function (got rid of some extra unnecessary else levels).  For the video portion, it checks for nsIDOMHTMLVideoElement, if so, casts it to a nsIVideoElement (safe?)... grabs videoWidth/videoHeight, creates a temporary surface, and calls Paint.

Comment 2

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

Some of this stuff should be #ifdef MOZ_MEDIA.

> For the video portion, it checks for nsIDOMHTMLVideoElement, if so, casts it to
> a nsIVideoElement (safe?)

Lies. You cast to nsHTMLVideoElement*. And it should be a static_cast... Safe, yes.

+        // we have to clear first, since some platform surfaces (X11, I'm
+        // looking at you) don't follow the cleared-surface convention.
+        ctx->SetOperator(gfxContext::OPERATOR_CLEAR);
+        ctx->Paint();

Can we get rid of this now that X11 surfaces do support the cleared-surface convention?

+        *aSurface = surf.forget().get();

surf.swap(*aSurface) seems a little cleaner.

Comment 3

[John Resig]

I haven't dug in to the patch yet - but is this going to be under the same URI/security restrictions that images are?

Comment 4

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to comment #2)
> Some of this stuff should be #ifdef MOZ_MEDIA.
> 
> > For the video portion, it checks for nsIDOMHTMLVideoElement, if so, casts it to
> > a nsIVideoElement (safe?)
> 
> Lies. You cast to nsHTMLVideoElement*. And it should be a static_cast... Safe,
> yes.

Erm, yes.

> +        // we have to clear first, since some platform surfaces (X11, I'm
> +        // looking at you) don't follow the cleared-surface convention.
> +        ctx->SetOperator(gfxContext::OPERATOR_CLEAR);
> +        ctx->Paint();
> 
> Can we get rid of this now that X11 surfaces do support the cleared-surface
> convention?

Yeah, need to verify that.

> +        *aSurface = surf.forget().get();
> 
> surf.swap(*aSurface) seems a little cleaner.

Yeah, though more confusing (IMO at least); I really wish I could just write '*aSurface = surf.forget();'


(In reply to comment #3)
> I haven't dug in to the patch yet - but is this going to be under the same
> URI/security restrictions that images are?

Yep; the restriction rules are effectively the same for both video and image.  You can draw any image, just can't read the image data back from images/video that are from a different origin.

Comment 5

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: Patch, v1

Updated, carrying forward review flags.

Comment 6

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Checked in, needs a test. (Oops.)