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

RESOLVED FIXED

Status

()

Core
Canvas: 2D
P3
normal
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: vlad, Assigned: vlad)

Tracking

Trunk
x86
All
Points:
---
Bug Flags:
wanted1.9.1 +
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

As the summary says -- <video> should be a valid source for drawImage; it should render whatever the current frame is.
Flags: wanted1.9.1+
Priority: -- → P3
Created attachment 331857 [details] [diff] [review]
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.
Attachment #331857 - Flags: superreview?(roc)
Attachment #331857 - Flags: review?(chris.double)

Updated

9 years ago
Attachment #331857 - Flags: review?(chris.double) → review+
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

9 years ago
I haven't dug in to the patch yet - but is this going to be under the same URI/security restrictions that images are?
(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.
Created attachment 332785 [details] [diff] [review]
Patch, v1

Updated, carrying forward review flags.
Attachment #331857 - Attachment is obsolete: true
Attachment #332785 - Flags: superreview?(roc)
Attachment #332785 - Flags: review+
Attachment #331857 - Flags: superreview?(roc)
Attachment #332785 - Flags: superreview?(roc) → superreview+
Checked in, needs a test. (Oops.)
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.