Closed Bug 448674 Opened 16 years ago Closed 16 years ago

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

Categories

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

x86
All
defect

Tracking

()

RESOLVED FIXED

People

(Reporter: vlad, Assigned: vlad)

Details

Attachments

(1 file, 1 obsolete file)

As the summary says -- <video> should be a valid source for drawImage; it should render whatever the current frame is.
Flags: wanted1.9.1+
Attached patch Patch, v0 (obsolete) — Splinter Review
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)
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.
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.
Attached patch Patch, v1Splinter Review
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
Closed: 16 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: