[html-in-canvas] Support video [chromium/src : main]
Stefan Zager (Gerrit)
Stefan Zager voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Philip Rogers
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stefan Zager (Gerrit)
Stefan Zager voted Code-Review+0![]( "Open in Gerrit")
| Code-Review | +0 |
Related details
- Philip Rogers
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Philip Rogers (Gerrit)
Philip Rogers added 2 comments![]( "Open in Gerrit")
background: green;nit: update this to either red or blue, and update the expectation below to either:
```
assert_array_equals(pixel, [255, 0, 0, 255], "Cross origin should not draw");
```
or
```
assert_array_equals(pixel, [0, 0, 255, 255], "Cross origin should not draw");
```
<video id=crossOrigin poster="https://{{hosts[alt][www]}}:{{ports[h2][0]}}/wpt_internal/html/canvas/drawElementImage/resources/red-100x100.png" >Can you update the h2 port to https?
(this may be the cause of the flakiness)
Related details
- Stefan Zager
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stefan Zager (Gerrit)
Stefan Zager voted and added 2 comments![]( "Open in Gerrit")
Votes added by Stefan Zager
| Commit-Queue | +1 |
2 comments
nit: update this to either red or blue, and update the expectation below to either:
```
assert_array_equals(pixel, [255, 0, 0, 255], "Cross origin should not draw");
```or
```
assert_array_equals(pixel, [0, 0, 255, 255], "Cross origin should not draw");
```
Done
<video id=crossOrigin poster="https://{{hosts[alt][www]}}:{{ports[h2][0]}}/wpt_internal/html/canvas/drawElementImage/resources/red-100x100.png" >Can you update the h2 port to https?
(this may be the cause of the flakiness)
- Philip Rogers
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Philip Rogers (Gerrit)
Philip Rogers added 7 comments![]( "Open in Gerrit")
High-level, I think this approach in this patch is too risky. The benefit vs patchset #6 is performance, but the performance of patchset #6 doesn't seem very bad to me, and a 1080p video plays smoothly in my debug browser. Is there an option of starting with something simpler at first, and then adding the faster approach as a flag-guarded followup?
[html-in-canvas] Support videoWDYT of adding the hardware-accelerated crash test to prove this patch prevents the crash (it does)? If so, you can patch in these files from https://chromium-review.git.corp.google.com/c/chromium/src/+/7798058/6:
- third_party/blink/web_tests/virtual/canvas-draw-element-image-hardware-video/README.txt
- third_party/blink/web_tests/virtual/canvas-draw-element-image-hardware-video/wpt_internal/html/canvas/drawElementImage/README.txt
- third_party/blink/web_tests/VirtualTestSuites
// This class must be created, used and destroyed on the same thread.Does this comment need updating given that the class inherits from RefCountedThreadSafe?
class VideoTextureBacking : public cc::TextureBacking {components/viz/common/gpu/raster_context_provider.h has the following:
```
// Get a Raster interface to the 3d context. The context provider must have
// been successfully bound to a thread before calling this.
virtual gpu::raster::RasterInterface* RasterInterface() = 0;
```
This implies a tight coupling between the context provider and the raster interface, and this patch is moving parts of this to happen on a different thread. This feels unsafe.
std::move(ri_access_), std::move(shared_image_)));Here we are moving ri_access_ so it will out-live raster_context_provider_. Do we also need to move raster_context_provider_? `ri_access_` has raw pointers to state that seems destructed (i.e., UAF risk).
~MailboxTextureBacking() override;I think the thread checker in this function will now crash, as this is run on a different thread. Does this class also need the task runner cleanup pattern?
<!DOCTYPE HTML>Can you add a worker thread version of this? I don't think that this feature is needed immediately, but I think it would be useful to ensure the lifetime changes in this patch are correct.
Here's a snippet (video-worker.html):
```
<!DOCTYPE HTML>
<html class="reftest-wait">
<head>
<title>captureElementImage repaints video frames in worker</title>
<meta name="fuzzy" content="maxDifference=0-2;totalPixels=0-4096">
<link rel="match" href="video-ref.html">
<script src="/common/reftest-wait.js"></script>
<script src='/wpt_internal/resources/canvas-draw-element/waitForCanvasPaint.js'></script>
<style>
#child {
width: 64px;
height: 64px;
}
#video {
width: 64px;
height: 64px;
}
</style>
</head>
<body>
<canvas id=canvas width="100" height="100" layoutsubtree>
<div id=child>
<video id=video src="resources/fast-red-green.webm" autoplay muted></video>
</div>
</canvas>
<script type="module">
let video = document.getElementById('video');
let canvas = document.getElementById('canvas');
let child = document.getElementById('child');
const workerCode = `
let ctx;
self.onmessage = (e) => {
if (e.data.canvas) {
ctx = e.data.canvas.getContext('2d');
}
if (e.data.elementImage) {
ctx.clearRect(0, 0, ctx.canvas.width, ctx.canvas.height);
ctx.drawElementImage(e.data.elementImage, 0, 0);
}
};
`;
const worker = new Worker(URL.createObjectURL(new Blob([workerCode], {type: 'application/javascript'})));
const offscreen = canvas.transferControlToOffscreen();
worker.postMessage({ canvas: offscreen }, [offscreen]);
await waitForCanvasPaint(canvas);
video.src = "resources/fast-red-green.webm";
video.onended = () => {
// Give the worker a tiny bit of time to draw the final frame.
setTimeout(takeScreenshot, 50);
};
canvas.onpaint = () => {
const elementImage = canvas.captureElementImage(child);
worker.postMessage({ elementImage: elementImage }, [elementImage]);
};
</script>
</body>
</html>
```This test currently crashes with:
```
STDERR: [31281:13738453:0511/094219.945249:FATAL:base/sequence_checker.cc:21] DCHECK failed: checker.CalledOnValidSequence(&bound_at).
STDERR: 0 base::debug::CollectStackTrace(base::span<void const*, 18446744073709551615ul, void const**>) + 84
STDERR: 1 base::debug::StackTrace::StackTrace(unsigned long) + 156
STDERR: 2 base::debug::StackTrace::StackTrace(unsigned long) + 36
STDERR: 3 base::debug::StackTrace::StackTrace() + 40
STDERR: 4 logging::LogMessage::Flush() + 184
STDERR: 5 logging::LogMessage::~LogMessage() + 44
STDERR: 6 logging::(anonymous namespace)::DCheckLogMessage::~DCheckLogMessage() + 72
STDERR: 7 logging::(anonymous namespace)::DCheckLogMessage::~DCheckLogMessage() + 28
STDERR: 8 logging::(anonymous namespace)::DCheckLogMessage::~DCheckLogMessage() + 28
STDERR: 9 std::__Cr::default_delete<logging::LogMessage>::operator()(logging::LogMessage*) const + 52
STDERR: 10 std::__Cr::unique_ptr<logging::LogMessage, std::__Cr::default_delete<logging::LogMessage>>::reset(logging::LogMessage*) + 68
STDERR: 11 logging::CheckError::~CheckError() + 60
STDERR: 12 logging::CheckError::~CheckError() + 28
STDERR: 13 base::ScopedValidateSequenceChecker::ScopedValidateSequenceChecker(base::SequenceCheckerImpl const&) + 296
STDERR: 14 base::ScopedValidateSequenceChecker::ScopedValidateSequenceChecker(base::SequenceCheckerImpl const&) + 36
STDERR: 15 viz::ContextProviderCommandBuffer::CheckValidSequenceOrLockAcquired() const + 100
STDERR: 16 viz::ContextProviderCommandBuffer::RasterInterface() + 184
STDERR: 17 media::VideoTextureBacking::readPixels(SkImageInfo const&, void*, unsigned long, int, int) + 64
STDERR: 18 media::VideoTextureBacking::GetSkImageViaReadback() + 152
STDERR: 19 cc::PaintImage::GetSwSkImage() const + 116
STDERR: 20 cc::PlaybackImageProvider::GetRasterContent(cc::DrawImage const&) + 696
STDERR: 21 blink::CanvasResourceProvider::CanvasImageProvider::GetRasterContent(cc::DrawImage const&) + 492
STDERR: 22 cc::DrawImageRectOp::RasterWithFlags(cc::DrawImageRectOp const*, cc::PaintFlags const*, SkCanvas*, cc::PlaybackParams const&) + 1228
STDERR: 23 cc::(anonymous namespace)::Rasterizer<cc::DrawImageRectOp, true>::RasterWithFlags(cc::DrawImageRectOp const*, cc::PaintFlags const*, SkCanvas*, cc::PlaybackParams const&) + 136
STDERR: 24 _ZNK2cc12_GLOBAL__N_15$_139clEPKNS_7PaintOpEPKNS_10PaintFlagsEP8SkCanvasRKNS_14PlaybackParamsE + 52
STDERR: 25 _ZN2cc12_GLOBAL__N_15$_1398__invokeEPKNS_7PaintOpEPKNS_10PaintFlagsEP8SkCanvasRKNS_14PlaybackParamsE + 68
STDERR: 26 cc::PaintOpWithFlags::RasterWithFlags(SkCanvas*, cc::PaintFlags const*, cc::PlaybackParams const&) const + 84
STDERR: 27 cc::PaintOpBuffer::Playback(SkCanvas*, cc::PlaybackParams const&, bool, std::__Cr::vector<unsigned long, std::__Cr::allocator<unsigned long>> const*) const + 1016
STDERR: 28 cc::PaintOpBuffer::Playback(SkCanvas*, cc::PlaybackParams const&, bool) const + 56
STDERR: 29 cc::PaintRecord::Playback(SkCanvas*, cc::PlaybackParams const&, bool) const + 56
STDERR: 30 cc::DrawRecordOp::Raster(cc::DrawRecordOp const*, SkCanvas*, cc::PlaybackParams const&) + 56
STDERR: 31 cc::(anonymous namespace)::Rasterizer<cc::DrawRecordOp, false>::Raster(cc::DrawRecordOp const*, SkCanvas*, cc::PlaybackParams const&) + 128
STDERR: 32 _ZNK2cc12_GLOBAL__N_14$_53clEPKNS_7PaintOpEP8SkCanvasRKNS_14PlaybackParamsE + 44
STDERR: 33 _ZN2cc12_GLOBAL__N_14$_538__invokeEPKNS_7PaintOpEP8SkCanvasRKNS_14PlaybackParamsE + 60
STDERR: 34 cc::PaintOp::Raster(SkCanvas*, cc::PlaybackParams const&) const + 76
STDERR: 35 cc::PaintOpBuffer::Playback(SkCanvas*, cc::PlaybackParams const&, bool, std::__Cr::vector<unsigned long, std::__Cr::allocator<unsigned long>> const*) const + 1112
STDERR: 36 cc::PaintOpBuffer::Playback(SkCanvas*, cc::PlaybackParams const&, bool) const + 56
STDERR: 37 cc::PaintRecord::Playback(SkCanvas*, cc::PlaybackParams const&, bool) const + 56
STDERR: 38 cc::DrawRecordOp::Raster(cc::DrawRecordOp const*, SkCanvas*, cc::PlaybackParams const&) + 56
STDERR: 39 cc::(anonymous namespace)::Rasterizer<cc::DrawRecordOp, false>::Raster(cc::DrawRecordOp const*, SkCanvas*, cc::PlaybackParams const&) + 128
STDERR: 40 _ZNK2cc12_GLOBAL__N_14$_53clEPKNS_7PaintOpEP8SkCanvasRKNS_14PlaybackParamsE + 44
STDERR: 41 _ZN2cc12_GLOBAL__N_14$_538__invokeEPKNS_7PaintOpEP8SkCanvasRKNS_14PlaybackParamsE + 60
STDERR: 42 cc::PaintOp::Raster(SkCanvas*, cc::PlaybackParams const&) const + 76
STDERR: 43 cc::PaintOpBuffer::Playback(SkCanvas*, cc::PlaybackParams const&, bool, std::__Cr::vector<unsigned long, std::__Cr::allocator<unsigned long>> const*) const + 1112
STDERR: 44 cc::PaintOpBuffer::Playback(SkCanvas*, cc::PlaybackParams const&, bool) const + 56
STDERR: 45 cc::PaintRecord::Playback(SkCanvas*, cc::PlaybackParams const&, bool) const + 56
STDERR: 46 cc::SkiaPaintCanvas::drawPicture(cc::PaintRecord, base::RepeatingCallback<void (SkCanvas*, unsigned int)>, bool) + 364
STDERR: 47 cc::SkiaPaintCanvas::drawPicture(cc::PaintRecord) + 96
STDERR: 48 blink::CanvasResourceProvider::UnacceleratedRasterRecordForCanvas2D(cc::PaintRecord) + 352
STDERR: 49 blink::Canvas2DResourceProviderSharedImage::RasterRecordForCanvas2D(cc::PaintRecord) + 108
STDERR: 50 blink::CanvasResourceProvider::FlushCanvas2D(blink::FlushReason) + 452
STDERR: 51 blink::OffscreenCanvasRenderingContext2D::FinalizeFrame(blink::FlushReason) + 152
STDERR: 52 blink::OffscreenCanvasRenderingContext2D::PushFrame() + 52
STDERR: 53 blink::OffscreenCanvas::PushFrameIfNeeded() + 728
```
Related details
- Stefan Zager
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |