Bug 149984 - Support createPattern(HTMLVideoElement...
Summary: Support createPattern(HTMLVideoElement...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Canvas (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dean Jackson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2015-10-09 19:04 PDT by Dean Jackson
Modified: 2016-06-06 13:28 PDT (History)
9 users (show)

See Also:


Attachments
Patch (12.41 KB, patch)
2016-05-26 15:51 PDT, George Ruan
no flags Details | Formatted Diff | Diff
In Progress (12.99 KB, patch)
2016-05-26 19:44 PDT, George Ruan
no flags Details | Formatted Diff | Diff
In Progress - Fixed Comments from Dean and Eric (12.94 KB, patch)
2016-05-26 19:50 PDT, George Ruan
no flags Details | Formatted Diff | Diff
Patch (220.85 KB, patch)
2016-05-31 20:15 PDT, George Ruan
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-yosemite (863.04 KB, application/zip)
2016-05-31 21:03 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews105 for mac-yosemite-wk2 (1.02 MB, application/zip)
2016-05-31 21:06 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews124 for ios-simulator-wk2 (669.95 KB, application/zip)
2016-05-31 21:11 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews117 for mac-yosemite (1.47 MB, application/zip)
2016-05-31 21:21 PDT, Build Bot
no flags Details
Patch (274.28 KB, patch)
2016-06-02 21:21 PDT, George Ruan
no flags Details | Formatted Diff | Diff
Patch (113.78 KB, patch)
2016-06-03 14:05 PDT, George Ruan
no flags Details | Formatted Diff | Diff
Patch (111.73 KB, patch)
2016-06-04 19:57 PDT, George Ruan
no flags Details | Formatted Diff | Diff
Patch (111.72 KB, patch)
2016-06-06 13:07 PDT, George Ruan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dean Jackson 2015-10-09 19:04:43 PDT
createPattern can accept an HTMLVideoElement
https://html.spec.whatwg.org/multipage/scripting.html#dom-context-2d-createpattern
Comment 1 Radar WebKit Bug Importer 2015-10-09 19:06:06 PDT
<rdar://problem/23058823>
Comment 2 George Ruan 2016-05-26 15:51:08 PDT
Created attachment 279917 [details]
Patch
Comment 3 Dean Jackson 2016-05-26 18:05:05 PDT
Comment on attachment 279917 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=279917&action=review

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1809
> +RefPtr<CanvasPattern> CanvasRenderingContext2D::createPattern(HTMLVideoElement* videoElement,
> +    const String& repetitionType, ExceptionCode& ec)

Style nit: Don't break the line here.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1833
> +    RefPtr<Image> currentFrame = imageBuffer->copyImage();
> +    
> +    return CanvasPattern::create(currentFrame, repeatX, repeatY, canvas()->originClean());

No need for the local variable.

> LayoutTests/media/video-canvas-createPattern.html:55
> +                expectedResults.forEach(function(element) {
> +                  checkColorAtLocation(ctx, element[0], element[1], element[2], element[3], element[4], 2);
> +                });

Nit: Four space indentation.
Comment 4 Eric Carlson 2016-05-26 19:43:11 PDT
Comment on attachment 279917 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=279917&action=review

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1829
> +    GraphicsContext& ctx = imageBuffer->context();

No need to declare this as a local variable either.

> LayoutTests/media/video-canvas-createPattern.html:60
> +                  video.currentTime += 0.24;
> +                  return;

Nit: please use four space indentation here as well.
Comment 5 George Ruan 2016-05-26 19:44:14 PDT
Created attachment 279937 [details]
In Progress
Comment 6 George Ruan 2016-05-26 19:50:27 PDT
Created attachment 279938 [details]
In Progress - Fixed Comments from Dean and Eric
Comment 7 Tim Horton 2016-05-26 19:56:56 PDT
Comment on attachment 279917 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=279917&action=review

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1828
> +    std::unique_ptr<ImageBuffer> imageBuffer = ImageBuffer::create(size(*videoElement), Unaccelerated);

Why explicitly Unaccelerated? It looks like paintCurrentFrameInContext sometimes uses a CVPixelBuffer-backed CGImage, and it may (should?) be more efficient to paint that into an accelerated ImageBuffer (and is *certainly* more efficient to paint from an accelerated ImageBuffer into the likely-accelerated canvas).

Seems like all of the surrounding code is a little wishy-washy about this, not just your new code, so this might be a future adjustment instead of something to fix in your patch.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1831
> +    RefPtr<Image> currentFrame = imageBuffer->copyImage();

If you decide that it's right to make the ImageBuffer sometimes accelerated, and since this is a short-lived ImageBuffer, you should use ImageBuffer::sinkIntoImage() here so that we can use the more efficient CreateImageReference.
Comment 8 George Ruan 2016-05-31 20:15:06 PDT
Created attachment 280205 [details]
Patch
Comment 9 George Ruan 2016-05-31 20:19:40 PDT
(In reply to comment #8)
> Created attachment 280205 [details]
> Patch

All of the security tests are literally the same except that the src and crossOrigin property differ.
Comment 10 Build Bot 2016-05-31 21:03:02 PDT
Comment on attachment 280205 [details]
Patch

Attachment 280205 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1415887

New failing tests:
fast/canvas/canvas-createPattern-video-invalid.html
media/video-canvas-createPattern.html
canvas/philip/tests/2d.pattern.image.null.html
Comment 11 Build Bot 2016-05-31 21:03:05 PDT
Created attachment 280206 [details]
Archive of layout-test-results from ews100 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 12 Build Bot 2016-05-31 21:06:29 PDT
Comment on attachment 280205 [details]
Patch

Attachment 280205 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1415891

New failing tests:
fast/canvas/canvas-createPattern-video-invalid.html
media/video-canvas-createPattern.html
canvas/philip/tests/2d.pattern.image.null.html
Comment 13 Build Bot 2016-05-31 21:06:32 PDT
Created attachment 280207 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5
Comment 14 Build Bot 2016-05-31 21:11:38 PDT
Comment on attachment 280205 [details]
Patch

Attachment 280205 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1415888

New failing tests:
fast/canvas/canvas-createPattern-video-invalid.html
canvas/philip/tests/2d.pattern.image.null.html
Comment 15 Build Bot 2016-05-31 21:11:42 PDT
Created attachment 280208 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.4
Comment 16 Build Bot 2016-05-31 21:21:16 PDT
Comment on attachment 280205 [details]
Patch

Attachment 280205 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1415920

New failing tests:
fast/canvas/canvas-createPattern-video-invalid.html
media/video-canvas-createPattern.html
canvas/philip/tests/2d.pattern.image.null.html
Comment 17 Build Bot 2016-05-31 21:21:20 PDT
Created attachment 280209 [details]
Archive of layout-test-results from ews117 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 18 Myles C. Maxfield 2016-05-31 21:31:20 PDT
Comment on attachment 280205 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=280205&action=review

> LayoutTests/fast/canvas/canvas-createPattern-video-invalid.html:7
> +<script src="script-tests/canvas-createPattern-video-invalid.js"></script>

Why not put the source of this script inline?

> LayoutTests/fast/canvas/canvas-createPattern-video-loading.html:7
> +<script src="script-tests/canvas-createPattern-video-loading.js"></script>

Ditto.

> LayoutTests/fast/canvas/canvas-createPattern-video-modify.html:7
> +<script src="script-tests/canvas-createPattern-video-modify.js"></script>

Ditto.

> LayoutTests/fast/canvas/script-tests/canvas-createPattern-video-invalid.js:5
> +shouldThrow("ctx.createPattern(undefined, undefined)", "'TypeError: Type error'");

This doesn't look right. Are you sure the second argument to shouldThrow should eval() to a string?
Comment 19 Myles C. Maxfield 2016-05-31 21:32:35 PDT
Comment on attachment 280205 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=280205&action=review

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1753
> +        ec = TypeError;

Looks like this is causing tests to fail. Not sure if we should update the test or revert this change. We should look in the spec.
Comment 20 Myles C. Maxfield 2016-05-31 21:34:30 PDT
Comment on attachment 280205 [details]
Patch

r- until you make the bots green
Comment 21 George Ruan 2016-06-02 21:21:03 PDT
Created attachment 280423 [details]
Patch
Comment 22 Chris Dumez 2016-06-02 21:23:43 PDT
(In reply to comment #19)
> Comment on attachment 280205 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=280205&action=review
> 
> > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1753
> > +        ec = TypeError;
> 
> Looks like this is causing tests to fail. Not sure if we should update the
> test or revert this change. We should look in the spec.

We should throw a TypeError, I have a patch to fix this at:
https://bugs.webkit.org/show_bug.cgi?id=158322
Comment 23 Chris Dumez 2016-06-02 21:43:32 PDT
Comment on attachment 280423 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=280423&action=review

> Source/WebCore/ChangeLog:3
> +        Support createPattern(HTMLVideoElement...

Why the ...?

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1825
> +    if (NativeImagePtr nativeImage = videoElement.nativeImageForCurrentTime()) {

Could use auto

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1826
> +        if (RefPtr<Image> image = BitmapImage::create(WTFMove(nativeImage)))

Could use auto. Pretty sure BitmapImage::create() returns a Ref<> not a RefPtr<>. Therefore, this null check is unnecessary as well.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1831
> +    std::unique_ptr<ImageBuffer> imageBuffer = ImageBuffer::create(size(videoElement), drawingContext() ? drawingContext()->renderingMode() : Accelerated);

Could use auto.

> LayoutTests/fast/canvas/canvas-createPattern-video-invalid.html:1
> +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">

<!DOCTYPE html>

> LayoutTests/fast/canvas/canvas-createPattern-video-invalid.html:8
> +    description("Test the handling of invalid arguments in canvas createPattern() with video.")

Not really related to video so this title and file name are a bit misleading.

> LayoutTests/fast/canvas/canvas-createPattern-video-invalid.html:10
> +    var ctx = document.createElement('canvas').getContext('2d');

ctx -> context
We prefer not to use abbreviations in WebKit.

> LayoutTests/fast/canvas/canvas-createPattern-video-loading.html:1
> +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">

<!DOCTYPE html>

> LayoutTests/fast/canvas/canvas-createPattern-video-loading.html:36
> +    shouldBe("document.createElement('canvas').getContext('2d').createPattern(video, 'repeat')", "null");

shouldBeNull()

> LayoutTests/fast/canvas/canvas-createPattern-video-loading.html:53
> +        var ctx = canvas.getContext("2d");

ctx -> context

> LayoutTests/fast/canvas/canvas-createPattern-video-modify.html:1
> +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">

Old Doctype.

> LayoutTests/fast/canvas/canvas-createPattern-video-modify.html:11
> +    var canvas, ctx;

Ctx -> context

> LayoutTests/fast/canvas/canvas-createPattern-video-modify.html:40
> +        else if (modified) {

Since we are in a else, the if(modified) is redundant.
Also, else should be on the same line as the closing curly bracket.

If (!modified) {
   ...
} else {
   ...
}

> LayoutTests/http/tests/security/canvas-remote-read-remote-video-allowed-anonymous-expected.txt:1
> +Untainted canvas:

Missing a description.

> LayoutTests/http/tests/security/canvas-remote-read-remote-video-allowed-anonymous.html:9
> +function log(msg)

Would be nice to write this using js-test as well.

> LayoutTests/http/tests/security/canvas-remote-read-remote-video-allowed-anonymous.html:55
> +    log("\n");

Log("") would probably suffice unless you need 2 blank lines for some reason.

> LayoutTests/http/tests/security/canvas-remote-read-remote-video-allowed-with-credentials-expected.txt:1
> +Untainted canvas:

Missing a description.

> LayoutTests/http/tests/security/canvas-remote-read-remote-video-allowed-with-credentials.html:9
> +function log(msg)

Would be nice to write this using js-test as well.

> LayoutTests/http/tests/security/canvas-remote-read-remote-video-allowed-with-credentials.html:55
> +    log("\n");

No need for the \n I believe.

> LayoutTests/http/tests/security/canvas-remote-read-remote-video-blocked-no-crossorigin-expected.txt:1
> +CONSOLE MESSAGE: line 18: Unable to get image data from canvas because the canvas has been tainted by cross-origin data.

Missing a description.

> LayoutTests/http/tests/security/canvas-remote-read-remote-video-blocked-no-crossorigin.html:2
> +<script src="../../media-resources/media-file.js"></script>

Same comments as for the other tests.

> LayoutTests/http/tests/security/canvas-remote-read-remote-video-localhost.html:9
> +function log(msg)

Same comments as for the other tests.

> LayoutTests/http/tests/security/canvas-remote-read-remote-video-redirect.html:9
> +function log(msg)

Same comments as for the other tests.

> LayoutTests/http/tests/security/canvas-remote-read-remote-video-redirect.html:75
> +video.src = "/resources/redirect.php?url=http://localhost:8000/security/resources/test.mp4";

There are a lot of mp4 files in the layout tests already. Is there any specific reason we need a new one?
Comment 24 George Ruan 2016-06-03 14:05:11 PDT
Created attachment 280461 [details]
Patch
Comment 25 Dean Jackson 2016-06-03 15:39:42 PDT
Comment on attachment 280461 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=280461&action=review

> LayoutTests/http/tests/security/canvas-remote-read-remote-video-allowed-anonymous.html:21
> +        // Control tests

Nit: WebKit coding style requires a full stop at the end of this comment.

> LayoutTests/http/tests/security/canvas-remote-read-remote-video-allowed-with-credentials.html:45
> +    function testVideo()
> +    {
> +        // Control tests
> +        log("Testing data retrieval on an untainted canvas:");
> +        canvas = document.createElement("canvas");
> +        canvas.width = 100;
> +        canvas.height = 100;
> +
> +        shouldNotThrow("canvas.getContext('2d').getImageData(0, 0, 100, 100)");
> +        shouldNotThrow("canvas.toDataURL()");
> +
> +        // Test CORS
> +        log("");
> +        
> +        log("Testing data retrieval on a canvas tainted by a remote video pattern:");
> +        canvas = document.createElement("canvas");
> +        canvas.width = 100;
> +        canvas.height = 100;
> +        context = canvas.getContext("2d");
> +        context.fillStyle = context.createPattern(video, "repeat");
> +        context.fillRect(0, 0, 100, 100);
> +
> +        shouldNotThrow("context.getImageData(0, 0, 100, 100)");
> +        shouldNotThrow("canvas.toDataURL()");
> +
> +        finishJSTest();
> +    }

It seems this function is the same in a few tests. Maybe you should split it out to a separate file?

> LayoutTests/media/video-canvas-createPattern.html:41
> +                    logResult(false, "Expected (" + r + ", " + g + ", " + b + ") at (" + x + ", " + y + ") but saw (" + buffer[0] + ", " + buffer[1] + ", " + buffer[2] + ")");

This line, and similar lines, can be made more readable by using ES6 string templates (or whatever they are called). `Expected (${r}, ${g}, ${b}) at (${x}, ${y}....`
Comment 26 Eric Carlson 2016-06-03 16:10:24 PDT
Comment on attachment 280461 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=280461&action=review

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1812
> +    if (videoElement.readyState() == HTMLMediaElement::HAVE_NOTHING || videoElement.readyState() == HTMLMediaElement::HAVE_METADATA)

This can be "if (videoElement.readyState() < HAVE_CURRENT_DATA)"

> LayoutTests/fast/canvas/canvas-createPattern-video-loading.html:38
> +    video.src = "../../media/content/test.mp4";

This should use "findMediaFile" because not all ports support MPEG-4.

> LayoutTests/fast/canvas/canvas-createPattern-video-modify.html:23
> +    video.src = "../../media/content/test.mp4";

This should use "findMediaFile" because not all ports support MPEG-4.

> LayoutTests/http/tests/security/canvas-remote-read-remote-video-allowed-anonymous.html:17
> +    function log(msg)
> +    {
> +        document.getElementById('console').appendChild(document.createTextNode(msg + "\n"));
> +    }

Is there any reason to not use "debug" from media-file.js?

> LayoutTests/http/tests/security/canvas-remote-read-remote-video-allowed-anonymous.html:33
> +        log("");
> +        
> +        log("Testing data retrieval on a canvas tainted by a remote video pattern:");

You can get rid of the first log by adding a "\n" (or "<br />" if you use debug) to the beginning of the string logged by the second.

> LayoutTests/http/tests/security/canvas-remote-read-remote-video-allowed-with-credentials.html:17
> +    function log(msg)
> +    {
> +        document.getElementById('console').appendChild(document.createTextNode(msg + "\n"));
> +    }

Is there any reason to not use "debug" from media-file.js?

> LayoutTests/http/tests/security/canvas-remote-read-remote-video-allowed-with-credentials.html:33
> +        log("");
> +        
> +        log("Testing data retrieval on a canvas tainted by a remote video pattern:");

You can get rid of the first log by adding a "\n" (or "<br />" if you use debug) to the beginning of the string logged by the second.

> LayoutTests/http/tests/security/canvas-remote-read-remote-video-blocked-no-crossorigin.html:17
> +    function log(msg)
> +    {
> +        document.getElementById('console').appendChild(document.createTextNode(msg + "\n"));
> +    }

Is there any reason to not use "debug" from media-file.js?

> LayoutTests/http/tests/security/canvas-remote-read-remote-video-blocked-no-crossorigin.html:32
> +        log("");
> +        

You can get rid of the first log by adding a "\n" (or "<br />" if you use debug) to the beginning of the string logged by the second.

> LayoutTests/http/tests/security/canvas-remote-read-remote-video-localhost.html:16
> +    function log(msg)
> +    {
> +        document.getElementById('console').appendChild(document.createTextNode(msg + "\n"));
> +    }

Is there any reason to not use "debug" from media-file.js?

> LayoutTests/http/tests/security/canvas-remote-read-remote-video-localhost.html:32
> +        log("");
> +        
> +        log("Testing data retrieval on a canvas tainted by a pattern generated by a localhost video resource:");

You can get rid of the first log by adding a "\n" (or "<br />" if you use debug) to the beginning of the string logged by the second.

> LayoutTests/http/tests/security/canvas-remote-read-remote-video-localhost.html:48
> +    video.src = "http://localhost:8080/resources/test.mp4";

This should use findMediaFile() as not all ports support MPEG-4.

> LayoutTests/http/tests/security/canvas-remote-read-remote-video-redirect.html:16
> +    function log(msg)
> +    {
> +        document.getElementById('console').appendChild(document.createTextNode(msg + "\n"));
> +    }

Is there any reason to not use "debug" from media-file.js?

> LayoutTests/http/tests/security/canvas-remote-read-remote-video-redirect.html:32
> +        log("");
> +        
> +        log("Testing data retrieval on a canvas tainted by a pattern generated by a redirected video resource:");

You can get rid of the first log by adding a "\n" (or "<br />" if you use debug) to the beginning of the string logged by the second.

> LayoutTests/http/tests/security/canvas-remote-read-remote-video-redirect.html:48
> +    video.src = "/resources/redirect.php?url=http://localhost:8000/resources/test.mp4";

This should use findMediaFile(), e.g. something like:

    video.src = `/resources/redirect.php?url=http://localhost:8000/${findMediaFile('video', 'resources/test')}`;

> LayoutTests/http/tests/security/resources/video-cross-origin-allow-credentials.php:10
> +header("Access-Control-Allow-Origin: http://127.0.0.1:8000");
> +header("Access-Control-Allow-Credentials: true");
> +
> +if ($_SERVER['REQUEST_METHOD'] == "OPTIONS") {
> +    header("Access-Control-Allow-Methods: GET");
> +    header("Access-Control-Allow-Headers: origin, accept-encoding, referer, range");
> +    exit;
> +}

It would be nice to add CORS support to serve-video.php with another url parameter so it can be reused in other tests. For example, look at the way that "content-length" or "Content-Type" are handled. 

If you would rather land this as-is and fix it in a follow-up, please file a bug for the change.
Comment 27 George Ruan 2016-06-04 19:57:10 PDT
Created attachment 280541 [details]
Patch
Comment 28 George Ruan 2016-06-04 20:02:32 PDT
(In reply to comment #26)
> Comment on attachment 280461 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=280461&action=review
> 
> > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1812
> > +    if (videoElement.readyState() == HTMLMediaElement::HAVE_NOTHING || videoElement.readyState() == HTMLMediaElement::HAVE_METADATA)
> 
> This can be "if (videoElement.readyState() < HAVE_CURRENT_DATA)"
> 
> > LayoutTests/fast/canvas/canvas-createPattern-video-loading.html:38
> > +    video.src = "../../media/content/test.mp4";
> 
> This should use "findMediaFile" because not all ports support MPEG-4.
> 
> > LayoutTests/fast/canvas/canvas-createPattern-video-modify.html:23
> > +    video.src = "../../media/content/test.mp4";
> 
> This should use "findMediaFile" because not all ports support MPEG-4.
> 
> > LayoutTests/http/tests/security/canvas-remote-read-remote-video-allowed-anonymous.html:17
> > +    function log(msg)
> > +    {
> > +        document.getElementById('console').appendChild(document.createTextNode(msg + "\n"));
> > +    }
> 
> Is there any reason to not use "debug" from media-file.js?
> 
> > LayoutTests/http/tests/security/canvas-remote-read-remote-video-allowed-anonymous.html:33
> > +        log("");
> > +        
> > +        log("Testing data retrieval on a canvas tainted by a remote video pattern:");
> 
> You can get rid of the first log by adding a "\n" (or "<br />" if you use
> debug) to the beginning of the string logged by the second.
> 
> > LayoutTests/http/tests/security/canvas-remote-read-remote-video-allowed-with-credentials.html:17
> > +    function log(msg)
> > +    {
> > +        document.getElementById('console').appendChild(document.createTextNode(msg + "\n"));
> > +    }
> 
> Is there any reason to not use "debug" from media-file.js?
> 
> > LayoutTests/http/tests/security/canvas-remote-read-remote-video-allowed-with-credentials.html:33
> > +        log("");
> > +        
> > +        log("Testing data retrieval on a canvas tainted by a remote video pattern:");
> 
> You can get rid of the first log by adding a "\n" (or "<br />" if you use
> debug) to the beginning of the string logged by the second.
> 
> > LayoutTests/http/tests/security/canvas-remote-read-remote-video-blocked-no-crossorigin.html:17
> > +    function log(msg)
> > +    {
> > +        document.getElementById('console').appendChild(document.createTextNode(msg + "\n"));
> > +    }
> 
> Is there any reason to not use "debug" from media-file.js?
> 
> > LayoutTests/http/tests/security/canvas-remote-read-remote-video-blocked-no-crossorigin.html:32
> > +        log("");
> > +        
> 
> You can get rid of the first log by adding a "\n" (or "<br />" if you use
> debug) to the beginning of the string logged by the second.
> 
> > LayoutTests/http/tests/security/canvas-remote-read-remote-video-localhost.html:16
> > +    function log(msg)
> > +    {
> > +        document.getElementById('console').appendChild(document.createTextNode(msg + "\n"));
> > +    }
> 
> Is there any reason to not use "debug" from media-file.js?
> 
> > LayoutTests/http/tests/security/canvas-remote-read-remote-video-localhost.html:32
> > +        log("");
> > +        
> > +        log("Testing data retrieval on a canvas tainted by a pattern generated by a localhost video resource:");
> 
> You can get rid of the first log by adding a "\n" (or "<br />" if you use
> debug) to the beginning of the string logged by the second.
> 
> > LayoutTests/http/tests/security/canvas-remote-read-remote-video-localhost.html:48
> > +    video.src = "http://localhost:8080/resources/test.mp4";
> 
> This should use findMediaFile() as not all ports support MPEG-4.
> 
> > LayoutTests/http/tests/security/canvas-remote-read-remote-video-redirect.html:16
> > +    function log(msg)
> > +    {
> > +        document.getElementById('console').appendChild(document.createTextNode(msg + "\n"));
> > +    }
> 
> Is there any reason to not use "debug" from media-file.js?
> 
> > LayoutTests/http/tests/security/canvas-remote-read-remote-video-redirect.html:32
> > +        log("");
> > +        
> > +        log("Testing data retrieval on a canvas tainted by a pattern generated by a redirected video resource:");
> 
> You can get rid of the first log by adding a "\n" (or "<br />" if you use
> debug) to the beginning of the string logged by the second.
> 
> > LayoutTests/http/tests/security/canvas-remote-read-remote-video-redirect.html:48
> > +    video.src = "/resources/redirect.php?url=http://localhost:8000/resources/test.mp4";
> 
> This should use findMediaFile(), e.g. something like:
> 
>     video.src =
> `/resources/redirect.php?url=http://localhost:8000/${findMediaFile('video',
> 'resources/test')}`;
> 
> > LayoutTests/http/tests/security/resources/video-cross-origin-allow-credentials.php:10
> > +header("Access-Control-Allow-Origin: http://127.0.0.1:8000");
> > +header("Access-Control-Allow-Credentials: true");
> > +
> > +if ($_SERVER['REQUEST_METHOD'] == "OPTIONS") {
> > +    header("Access-Control-Allow-Methods: GET");
> > +    header("Access-Control-Allow-Headers: origin, accept-encoding, referer, range");
> > +    exit;
> > +}
> 
> It would be nice to add CORS support to serve-video.php with another url
> parameter so it can be reused in other tests. For example, look at the way
> that "content-length" or "Content-Type" are handled. 
> 
> If you would rather land this as-is and fix it in a follow-up, please file a
> bug for the change.

New bug regarding CORS support for serve-video.php: 26639207.
Comment 29 Darin Adler 2016-06-05 10:41:17 PDT
Comment on attachment 280541 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=280541&action=review

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1805
> +    if (videoElement.readyState() < HTMLMediaElement::HAVE_CURRENT_DATA)
> +        return nullptr;

No exception in this case?

> Source/WebCore/html/canvas/CanvasRenderingContext2D.idl:180
> +    [Conditional=VIDEO, RaisesException] CanvasPattern createPattern(HTMLVideoElement video, [TreatNullAs=EmptyString] DOMString repetitionType);

Given the fact that this returns null when the video doesn’t have data, the return type should be nullable: "CanvasPattern?" instead of "CanvasPattern".

Or maybe we should be raising an exception in that case. In which case the return type would remain, but we would have to set ec above.
Comment 30 Dean Jackson 2016-06-05 13:39:37 PDT
Comment on attachment 280541 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=280541&action=review

>> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1805
>> +        return nullptr;
> 
> No exception in this case?

No. The specification says:

  If the image argument is an HTMLOrSVGImageElement object that is not fully decodable, or if the image argument is an HTMLVideoElement object whose readyState attribute is either HAVE_NOTHING or HAVE_METADATA, then return bad and abort these steps.

Since HAVE_NOTHING and HAVE_METADATA are the only two values less than HAVE_CURRENT_DATA this is correct.

>> Source/WebCore/html/canvas/CanvasRenderingContext2D.idl:180
>> +    [Conditional=VIDEO, RaisesException] CanvasPattern createPattern(HTMLVideoElement video, [TreatNullAs=EmptyString] DOMString repetitionType);
> 
> Given the fact that this returns null when the video doesn’t have data, the return type should be nullable: "CanvasPattern?" instead of "CanvasPattern".
> 
> Or maybe we should be raising an exception in that case. In which case the return type would remain, but we would have to set ec above.

Indeed! Great catch.

In fact, all the createPattern calls are defined to return  CanvasPattern?

So George, please do this for the createPattern you are adding, and we'll fix the rest in a followup bug.
Comment 31 George Ruan 2016-06-06 13:07:26 PDT
Created attachment 280621 [details]
Patch
Comment 32 WebKit Commit Bot 2016-06-06 13:28:40 PDT
Comment on attachment 280621 [details]
Patch

Clearing flags on attachment: 280621

Committed r201724: <http://trac.webkit.org/changeset/201724>
Comment 33 WebKit Commit Bot 2016-06-06 13:28:47 PDT
All reviewed patches have been landed.  Closing bug.