Currently render video-to-canvas2D does color space conversion on CPU, which is slow on B2G. Use GPU to do conversion to a GraphicBuffer can save CPU power and enhance the performance.

Here is a WIP patch. This patch is fairly large and complicated than expected since I have to save/restore tons of GL states to make JS side not ware any change under the table. A clearer way to make this is to create a clean shared context and convert the texture there. However, it will use more resource than needed.

This patch only tested with PlanarYCbCr images, GrallocImage case is under testing.

bjacob, jgilbert, I tested this patch against cubevid, and both GrallocImage case and PlanarYCbCrImage case works well, do you think I miss anything?

(In reply to Chiajung Hung [:chiajung] from comment #3) > bjacob, jgilbert, > > I tested this patch against cubevid, and both GrallocImage case and > PlanarYCbCrImage case works well, do you think I miss anything? Is this bug for canvas2d or webgl? Because the bug talks about one, and the patch is for the other.

Also, we want to do most format conversions on the GPU, so I'd prefer if we just folded this usecase into that work item. (bug 750994) In general, there's no reason this should be attached to WebGLContext. It should really go in GLContextUtils, like the existing texture/framebuffer blit code.

Sorry, this one is for webgl..(In reply to Jeff Gilbert [:jgilbert] from comment #5) > Also, we want to do most format conversions on the GPU, so I'd prefer if we > just folded this usecase into that work item. (bug 750994) > > In general, there's no reason this should be attached to WebGLContext. It > should really go in GLContextUtils, like the existing texture/framebuffer > blit code. There would be some problem to implement this without a new GLContext in Utils. For example, if webapp side enable some vertexAttribArray before texImage calls, we have no way to get such information, since there is no way to query such infomation.

(In reply to Chiajung Hung [:chiajung] from comment #6) > Sorry, this one is for webgl..(In reply to Jeff Gilbert [:jgilbert] from > comment #5) > > Also, we want to do most format conversions on the GPU, so I'd prefer if we > > just folded this usecase into that work item. (bug 750994) > > > > In general, there's no reason this should be attached to WebGLContext. It > > should really go in GLContextUtils, like the existing texture/framebuffer > > blit code. > There would be some problem to implement this without a new GLContext in > Utils. For example, if webapp side enable some vertexAttribArray before > texImage calls, we have no way to get such information, since there is no > way to query such infomation. Or you have to loop through all vertexAttribute with fGetVertexAttribiv...

I will try to move this to GLContextUtil in next version.

Move to GLContextUtil and fix memory leakage. CubeVid test result on Unagi(PlanarYCbCrImage case(software decoder), top result): Before: Main Thread: 80% FPS: 6 After: Main Thread: 20% Decoder Thread: 60% FPS: 12

I previous attach the patch to wrong bug, and make there multiple bugs for webGL case. Change this bug back to 2d canvas case now. Sorry for the confusion.

In fact, I think video-to-skiaGL path is harder to implement than video-to-WebGL case. I will implement webGL part(bug 814524) first.

Test results with Firefox 28.0 Test machine: Ubuntu 13.04 kernel 3.8.0-35, Driver: Nvidia driver 331.20, GPU: GTX-780, CPU: Intel Core i7 4770S 3.1ghz Full Test ========= Test program (WebGL, JS): http://codeflow.org/issues/slow_video_to_texture/ (archive http://codeflow.org/issues/slow_video_to_texture.zip) Results: detailed saved under historical on the test page, summary quoted below. * Minor difference between texImage2D and texSubImage2D speed * 240p texSubImage2D was not rendering and had anomalous timings, whereas texImage2D was properly rendering * H.264 was a bit faster than VP8 or Theora * 240p avg (excluding anomaly): 0.44ms/frame, 25% worse than Firefox 27.0.1 (0.35ms/frame) * 480p avg: 1.95ms/frame, 4% worse than Firefox 27.0.1 (1.85ms/frame) * 720p avg: 3.99ms/frame, 10% better than Firefox 27.0.1 (4.45ms/frame) * 1080p avg: 8.15ms/frame, 16% better than Firefox 27.0.1 (9.76ms/frame) Simple Test =========== Test program (WebGL, JS): http://codeflow.org/issues/slow_video_to_texture/simple.html (archive http://codeflow.org/issues/slow_video_to_texture.zip) Results: * FPS: 48.09 * Upload 9.04ms Theoretical Best results ======================== Test program (GL 4.4, C++): http://codeflow.org/issues/slow_video_to_texture/video-test.zip Test method: raw yuv420p frames shuffled to the GPU where they are decoded DryRun no upload activity: 1080p: 0.17ms 720p: 0.10ms 480p: 0.06ms DryRun memcpy of the video frame to heap: 1080p: 0.29ms 720p: 0.11ms 480p: 0.06ms TexSubImage2D one texture each for Y, U and V 1080p: 0.79ms 720p: 0.34ms 480p: 0.18ms Double buffered PBO, one texture each for Y, U and V 1080p: 0.58ms 720p: 0.31ms 480p: 0.19ms Single TBO (texture buffer object) and glBufferSubData for update 1080p: 0.69ms 720p: 0.33ms 480p: 0.18ms Single TBO and persistent mapped buffer for update 1080p: 0.74ms 720p: 0.32ms 480p: 0.15ms Conclusion ========== Comparing full test results to theoretical best performance for simple upload case of TexSubImage2D * 480p: 983% overhead (1.77ms/0.18ms) * 720p: 1073% overhead (3.65ms/0.34ms) * 1080p: 931% overhead (7.36ms/0.79ms)

Anyone want to try this patch must apply the patch in 814524 first. This is a very simple version to show it works. However, the performance is not very good, which is under tuning. And the final solution for this case would be render the video frame into GLContext directly, which need take care of Matrix, blending, src/dst crop directly. A side effect of this patch is that if a GraphicBuffer can be composed in Compositor directly, the color space conversion is always correct, which means we can forget the GrallocImages::GetAsSurface and the platform dependent color space conversion there.

Faster and seems not leak. SkiaGL deferred all rendering call until flush make this implementation very fast. The WebGL case is much more slower on B2G devices(deferred GPU) since it may sample the RGB texture as soon as TexImage2D calls, and make CPU wait for conversion(render-to-texture) done. This one make http://www.craftymind.com/factory/html5video/CanvasVideo.html runs quiet smooth on Unagi except it sometimes causes genlock fail.

It seems the video deadlock not related to my patch, the deadlock presents even without my patch.

Comment on attachment 8409594 [details] [diff] [review] V1 Review of attachment 8409594 [details] [diff] [review]: ----------------------------------------------------------------- I would prefer to move the GPU-based color space conversion modification to [1] with *GLContext as input, like GrallocImage::GetAsSourceSurface(GLContext* gl). If there is no GLContext then, you can create it inside GetAsSourceSurface(). For canvas 2D, you can configure skiaGL with this sourcesurface from GrallocImage. BTW, please consider to provide the read back capability for non-GL usage. How do you think? [1]http://dxr.mozilla.org/mozilla-central/source/gfx/layers/GrallocImages.cpp?from=GrallocImages.cpp&case=true#187

Comment on attachment 8409594 [details] [diff] [review] V1 Review of attachment 8409594 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/canvas/src/CanvasRenderingContext2D.cpp @@ +3289,5 @@ > + skiaSurf->InitFromVideo(video, gfxPlatform::GetPlatform()->GetSkiaGLGlue()->GetGLContext(), gfxPlatform::GetPlatform()->GetSkiaGLGlue()->GetGrContext(), SurfaceFormat::R8G8B8A8, &mStreamingTexture); > + imgSize.width = skiaSurf->GetSize().width; > + imgSize.height = skiaSurf->GetSize().height; > + srcSurf = skiaSurf; > + } Can we move this code segment(#3285 ~ 3293) into nsLayoutUtils::SurfaceFromElement(HTMLVideoElement* aElement...)? ::: gfx/2d/SourceSurfaceSkia.cpp @@ +95,5 @@ > > +bool SourceSurfaceSkia::InitFromVideo(mozilla::dom::HTMLVideoElement* video, > + mozilla::gl::GLContext* aGL, > + GrContext* aGr, > + SurfaceFormat aFormat, Why need aFormat? @@ +117,5 @@ > + return false; > + } > + uint dstTexture = 0; > + if (aStreamingTexture && *aStreamingTexture) > + dstTexture = *aStreamingTexture; Why need to return created streaming texture to caller? Can't we just hold this streaming texture in SourceSurfaceSkia and make caller totally transparent in it?

Nominate this to 2.0 for the fix since this will also resole the issue on the vendor dependent code pixel format.

Comment on attachment 8409594 [details] [diff] [review] V1 Review of attachment 8409594 [details] [diff] [review]: ----------------------------------------------------------------- I will fix most of them and upload new version later. Thanks ::: content/canvas/src/CanvasRenderingContext2D.cpp @@ +3289,5 @@ > + skiaSurf->InitFromVideo(video, gfxPlatform::GetPlatform()->GetSkiaGLGlue()->GetGLContext(), gfxPlatform::GetPlatform()->GetSkiaGLGlue()->GetGrContext(), SurfaceFormat::R8G8B8A8, &mStreamingTexture); > + imgSize.width = skiaSurf->GetSize().width; > + imgSize.height = skiaSurf->GetSize().height; > + srcSurf = skiaSurf; > + } No... I think nsLayoutUtil should independent with GLContext. Though I know it's better in structure, but the problem is the simple shader I have does not handle all other tedious format/premultiply alpha/rb-swap cases, thus I can not fully replace the SurfaceFromElement series functions by GPU code now. Moreover, we should be able to fallback to software path if needed, that's why the next if(!srcSurf) exists. And this remind me that I forget to check InitFromVideo return value before assign skiaSurf into srcSurf. ::: gfx/2d/SourceSurfaceSkia.cpp @@ +95,5 @@ > > +bool SourceSurfaceSkia::InitFromVideo(mozilla::dom::HTMLVideoElement* video, > + mozilla::gl::GLContext* aGL, > + GrContext* aGr, > + SurfaceFormat aFormat, In fact, this is for output format I forget to use in this function... @@ +117,5 @@ > + return false; > + } > + uint dstTexture = 0; > + if (aStreamingTexture && *aStreamingTexture) > + dstTexture = *aStreamingTexture; return? In fact, I forgot to return blit result to caller. :p

(In reply to peter chang[:pchang][:peter] from comment #17) > Comment on attachment 8409594 [details] [diff] [review] > V1 > > Review of attachment 8409594 [details] [diff] [review]: > ----------------------------------------------------------------- > > I would prefer to move the GPU-based color space conversion modification to > [1] with *GLContext as input, like > GrallocImage::GetAsSourceSurface(GLContext* gl). > If there is no GLContext then, you can create it inside GetAsSourceSurface(). > For canvas 2D, you can configure skiaGL with this sourcesurface from > GrallocImage. > > BTW, please consider to provide the read back capability for non-GL usage. > > How do you think? > > [1]http://dxr.mozilla.org/mozilla-central/source/gfx/layers/GrallocImages. > cpp?from=GrallocImages.cpp&case=true#187 This bug is filed for performance enhancement, let's file another bug for non-GL paths GPU conversion if needed. (I think most use case we have should be covered by this patch and CompositorOGL backed DrawWindow, so we may need find a use case for that first :p)

Discussed offline with :milan, this is at best marked targeting feature-b2g:2.0 .But as it stands today this is not a blocker for shipping 2.0. In case we are having a low risk solution available which misses the FL date by a few days please seek gecko/gaia approval and we can consider uplift.

Many small fixes, tested on Flame with: http://www.craftymind.com/factory/html5video/CanvasVideo.html goes from 15~20fps to 30fps. Unagi has some strange problem: this test case has 2 canvas: canvas A: used to draw video frame canvas B: draw canvas A onto it and present to screen Canvas A runs skiaGL and canvas B runs skia on unagi, canvas B shows nothing with my patch. However, if I force them run skiaGL, everything works well. @snorp Do you know any possible problem of skia/skiaGL interoperation?

(In reply to Chiajung Hung [:chiajung] from comment #23) > Created attachment 8433805 [details] [diff] [review] > V2 WIP > > Many small fixes, tested on Flame with: > > http://www.craftymind.com/factory/html5video/CanvasVideo.html > > goes from 15~20fps to 30fps. Unagi has some strange problem: this test case > has 2 canvas: > canvas A: used to draw video frame > canvas B: draw canvas A onto it and present to screen > > Canvas A runs skiaGL and canvas B runs skia on unagi, canvas B shows nothing > with my patch. However, if I force them run skiaGL, everything works well. > > @snorp > Do you know any possible problem of skia/skiaGL interoperation? Drawing from a GL canvas into a software one will require a GL readback, which is known to be slow.

Comment on attachment 8433805 [details] [diff] [review] V2 WIP Review of attachment 8433805 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/canvas/src/CanvasRenderingContext2D.cpp @@ +879,4 @@ > gScreenPixels = std::max(gScreenPixels, width * height); > } > } > + //gScreenPixels = 320*240; remove @@ +930,4 @@ > nsContentUtils::PersistentLayerManagerForDocument(ownerDoc); > } > > + mUseGPUBackend = false; Please use DrawTargetSkia::UsingSkiaGPU @@ +3292,5 @@ > + if (!mStreamingTexture) { > + gl->fGenTextures(1, &mStreamingTexture); > + } > + > + nsRefPtr<SourceSurfaceSkia> skiaSurf = new SourceSurfaceSkia(); I think you can create similar API from DrawTargetSkia::CreateSourceSurfaceFromData and return SourceSurfaceSKia. BTW, it is possible to reuse this "skiaSurf" when this source is video. And then you don't need to save video width and height @@ +3307,5 @@ > // The canvas spec says that drawImage should draw the first frame > // of animated images. We also don't want to rasterize vector images. > uint32_t sfeFlags = nsLayoutUtils::SFE_WANT_FIRST_FRAME | > nsLayoutUtils::SFE_NO_RASTERIZING_VECTORS; > + remove new line ::: gfx/2d/DrawTargetSkia.cpp @@ +338,5 @@ > > mCanvas->drawBitmapRectToRect(bitmap.mBitmap, &sourceRect, destRect, &paint.mPaint); > + > + SkBitmap test; > + mCanvas->readPixels(&test, 0, 0); what's this for? ::: gfx/2d/SourceSurfaceSkia.cpp @@ +56,3 @@ > SkISize size = aCanvas->getDeviceSize(); > > mBitmap = (SkBitmap)aCanvas->getDevice()->accessBitmap(false); remove new line @@ +133,5 @@ > + return texInfo; > +} > + > +GrPixelConfig SkPixelConfigFromTextureFormat(TextureFormat texInfo) > +{ Using TextureFormatToGrPixelConfig is simpler @@ +150,5 @@ > + return kUnknown_GrPixelConfig; > +} > + > +SkColorType SkColorTypeFromGrPixelConfig(GrPixelConfig grconfig) > +{ GrPixelConfigToSkColorType @@ +199,5 @@ > + if (!aStreamingTexture) { > + return false; > + } > + > + nsRefPtr<mozilla::layers::Image> srcImage = container->LockCurrentImage(); Suggest to pass layers::Image as Input and no need to add ImageContainer and HTMLVideo element to DrawTargetSkia. How do you think? @@ +233,5 @@ > + GrTexture *skiaTexture = aGr->wrapBackendTexture(skiaTexGlue); > + SkImageInfo imgInfo = SkImageInfo::Make(srcImage->GetSize().width, srcImage->GetSize().height, SkColorTypeFromGrPixelConfig(skiaTexGlue.fConfig), kOpaque_SkAlphaType); > + SkGrPixelRef *texRef = new SkGrPixelRef(imgInfo, skiaTexture, false); > + mBitmap.setPixelRef(texRef); > + Please add comment @@ +246,5 @@ > + unsigned char* data = new unsigned char [srcImage->GetSize().width * srcImage->GetSize().height * 4]; > + aGL->fReadPixels(0, 0, srcImage->GetSize().width, srcImage->GetSize().height, LOCAL_GL_RGBA, LOCAL_GL_UNSIGNED_BYTE, data); > + mBitmap.setConfig(SkBitmap::kARGB_8888_Config, srcImage->GetSize().width, srcImage->GetSize().height, srcImage->GetSize().width*4); > + mBitmap.setPixels(data); > + aGL->fDeleteFramebuffers(1, &mFBO);*/ remove above code @@ +249,5 @@ > + mBitmap.setPixels(data); > + aGL->fDeleteFramebuffers(1, &mFBO);*/ > + > + container->UnlockCurrentImage(); > + return true; use variable to cache the result and then you don't need to have this return call.

Comment on attachment 8433805 [details] [diff] [review] V2 WIP Review of attachment 8433805 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the suggestions. I will upload new version later. ::: content/canvas/src/CanvasRenderingContext2D.cpp @@ +879,4 @@ > gScreenPixels = std::max(gScreenPixels, width * height); > } > } > + //gScreenPixels = 320*240; Done. @@ +930,4 @@ > nsContentUtils::PersistentLayerManagerForDocument(ownerDoc); > } > > + mUseGPUBackend = false; mTarget is DrawTarget not DrawTargetSkia...Maybe I can hoist up InitFromCanvas to DrawTarget and move the logic to DrawTargetSkia. @@ +3292,5 @@ > + if (!mStreamingTexture) { > + gl->fGenTextures(1, &mStreamingTexture); > + } > + > + nsRefPtr<SourceSurfaceSkia> skiaSurf = new SourceSurfaceSkia(); Done. @@ +3307,5 @@ > // The canvas spec says that drawImage should draw the first frame > // of animated images. We also don't want to rasterize vector images. > uint32_t sfeFlags = nsLayoutUtils::SFE_WANT_FIRST_FRAME | > nsLayoutUtils::SFE_NO_RASTERIZING_VECTORS; > + Done. ::: gfx/2d/DrawTargetSkia.cpp @@ +338,5 @@ > > mCanvas->drawBitmapRectToRect(bitmap.mBitmap, &sourceRect, destRect, &paint.mPaint); > + > + SkBitmap test; > + mCanvas->readPixels(&test, 0, 0); For test...Removed. ::: gfx/2d/SourceSurfaceSkia.cpp @@ +56,3 @@ > SkISize size = aCanvas->getDeviceSize(); > > mBitmap = (SkBitmap)aCanvas->getDevice()->accessBitmap(false); Done. @@ +133,5 @@ > + return texInfo; > +} > + > +GrPixelConfig SkPixelConfigFromTextureFormat(TextureFormat texInfo) > +{ Done. @@ +150,5 @@ > + return kUnknown_GrPixelConfig; > +} > + > +SkColorType SkColorTypeFromGrPixelConfig(GrPixelConfig grconfig) > +{ Done. @@ +199,5 @@ > + if (!aStreamingTexture) { > + return false; > + } > + > + nsRefPtr<mozilla::layers::Image> srcImage = container->LockCurrentImage(); Basically, they are identical. Anyway, someone must check such conditions somewhere, I prefer check right before using it. @@ +246,5 @@ > + unsigned char* data = new unsigned char [srcImage->GetSize().width * srcImage->GetSize().height * 4]; > + aGL->fReadPixels(0, 0, srcImage->GetSize().width, srcImage->GetSize().height, LOCAL_GL_RGBA, LOCAL_GL_UNSIGNED_BYTE, data); > + mBitmap.setConfig(SkBitmap::kARGB_8888_Config, srcImage->GetSize().width, srcImage->GetSize().height, srcImage->GetSize().width*4); > + mBitmap.setPixels(data); > + aGL->fDeleteFramebuffers(1, &mFBO);*/ Done. @@ +249,5 @@ > + mBitmap.setPixels(data); > + aGL->fDeleteFramebuffers(1, &mFBO);*/ > + > + container->UnlockCurrentImage(); > + return true; Done.

Slightly refactor related code.

Comment on attachment 8439047 [details] [diff] [review] V3 Review of attachment 8439047 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/canvas/src/CanvasRenderingContext2D.cpp @@ +107,5 @@ > #include "SurfaceTypes.h" > #endif > > +#ifdef USE_SKIA_GPU > +#include "mozilla/TimeStamp.h" No need for TimeStamp.h @@ +558,4 @@ > > CanvasRenderingContext2D::CanvasRenderingContext2D() > : mForceSoftware(false), mZero(false), mOpaque(false), mResetLayer(true) > + , mStreamingTexture(0), mCurrentVideoWidth(0), mCurrentVideoHeight(0) remove mCurrentVideoWidth/mCurrentVideoHeight/mStreamingTexture ::: content/canvas/src/CanvasRenderingContext2D.h @@ +682,5 @@ > + // Texture object for streaming from outside > + unsigned int mStreamingTexture; > + unsigned int mCurrentVideoWidth; > + unsigned int mCurrentVideoHeight; > + remove mCurrentVideoWidth and mCurrentVideoHeight ::: gfx/2d/2D.h @@ +866,5 @@ > int32_t aStride, > SurfaceFormat aFormat) const = 0; > > + virtual TemporaryRef<SourceSurface> CreateSourceSurfaceFromVideo(dom::HTMLVideoElement* aVideo, > + SurfaceFormat aFormat) Change 2D API, you need to discuss with Bas first. ::: gfx/2d/DrawTargetSkia.cpp @@ +642,4 @@ > return result; > } > > +TemporaryRef<SourceSurface> DrawTargetSkia::CreateSourceSurfaceFromVideo(dom::HTMLVideoElement* aVideo, pass video by reference @@ +652,5 @@ > + if (!mCachedVideoSurface) { > + mCachedVideoSurface = new SourceSurfaceSkia(); > + } > + > + if (mCachedVideoSurface->InitFromVideo(*aVideo, gl, mGlue->GetGrContext(), pass video by reference ::: gfx/2d/SourceSurfaceSkia.cpp @@ +25,3 @@ > SourceSurfaceSkia::SourceSurfaceSkia() > : mDrawTarget(nullptr), mLocked(false) > { initialize mGL as nullptr @@ +108,5 @@ > + TextureFormat() { > + format = type = 0; > + valid = false; > + } > +}; Can we merge the format stuff with HelpersSkia? @@ +177,5 @@ > +} > + > +bool SourceSurfaceSkia::InitFromVideo(mozilla::dom::HTMLVideoElement& video, > + mozilla::gl::GLContext* aGL, > + GrContext* aGr, Since you need GL and GrContext, consider to pass GLUE @@ +182,5 @@ > + SurfaceFormat aFormat) > +{ > +#ifdef USE_SKIA_GPU > + MOZ_ASSERT(aGL, "GLContext needed"); > + mGL = aGL; return error if mGL is nullptr

Comment on attachment 8439047 [details] [diff] [review] V3 Review of attachment 8439047 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/2d/SourceSurfaceSkia.cpp @@ +11,4 @@ > #include "HelpersSkia.h" > #include "DrawTargetSkia.h" > #include "DataSurfaceHelpers.h" > +#include "mozilla/dom/HTMLVideoElement.h" Moz2D shouldn't have a dependency on Gecko stuff. i.e. it needs to be built stand alone. You're going to need to rework this to avoid that.

NI: Ivan as per comment comment #22 I think this is best to be resolved in 2.1 given its not landed yet. Can you help here ?

(In reply to bhavana bajaj [:bajaj] [NOT reading Bugmail, needInfo please] from comment #30) > NI: Ivan as per comment comment #22 I think this is best to be resolved in > 2.1 given its not landed yet. Can you help here ? Originally this bug is to resolve the issue in bug 931733. After discussing this with Peter, we can move this to backlog since there is already another solution to fix the issue in bug 931733 with the lower risk and less development time. For this bug, move it to backlog and block graphic backlog meta bug. But will keep developing it for the future release. The patch in this bug will take longer time for the development and review. As a resulr, this bug no longer blocks bug 931733 and it is also not in the feature scope of v2.0 and 2.1 for now.

Retested with Chrome 36.0.1985.125, Firefox Aurora 33.0a2, Safari 6.1.4 and Internet Explorer 11 TL;DR Firefox now outperforms Chrome by around 600% on OSX and Linux. Summary of result tables, for full results please see: http://codeflow.org/issues/slow_video_to_texture/ Ubuntu: FPS UPms FPS UPms FPS UPms Chrome 36 1080p 46.42 8.08 46.22 7.97 46.72 7.80 Chrome 35 1080p 45.28 8.02 45.10 8.03 45.39 7.98 Firefox Aurora 1080p 60.04 1.00 60.04 1.14 60.02 1.18 OSX: FPS UPms FPS UPms FPS UPms Chrome 36 1080p 59.56 9.91 59.72 9.96 59.78 9.91 Chrome 35 1080p 59.77 13.47 58.88 14.51 59.85 13.69 Firefox Aurora 1080p N/A N/A 59.50 1.54 59.07 1.50 Safari 6.1.4 1080p 20.07 34.94 N/A N/A N/A N/A Windows 8.1: FPS UPms FPS UPms FPS UPms Chrome 36 1080p 51.22 11.53 59.53 7.16 59.23 7.23 Firefox Aurora 1080p 57.14 9.55 56.89 9.41 IE11 video not supported for upload Conclusion: Firefox has made good efforts in its Aurora build (scheduled to be beta early September, and release early October) on OSX and Linux, these now outperform Chrome by a substantial margin (by ~600%) and pushed upload times into the range of 1-2ms. Chrome 36 has also made some gains on Chrome 35 on OSX (by ~45%). Firefox has yet to implement their improvements on Windows and has also got to deal with bugs in their h.264 support on Windows. Firefox issues: - The optimization from this ticket https://bugzilla.mozilla.org/show_bug.cgi?id=814524 has not made it to the Firefox Aurora windows build. Is this a new error? Should I open a ticket for it? Or is this tracked by this ticket? - The H.264 implementation of Firefox is buggy. It's quite jittery and often crashes (I have not looked for a ticket for this yet, is this known?)

(In reply to Florian Bösch from comment #33) > Retested with Chrome 36.0.1985.125, Firefox Aurora 33.0a2, Safari 6.1.4 and > Internet Explorer 11 > > TL;DR Firefox now outperforms Chrome by around 600% on OSX and Linux. > > Summary of result tables, for full results please see: > http://codeflow.org/issues/slow_video_to_texture/ > > Ubuntu: > FPS UPms FPS UPms FPS UPms > Chrome 36 1080p 46.42 8.08 46.22 7.97 46.72 7.80 > Chrome 35 1080p 45.28 8.02 45.10 8.03 45.39 7.98 > Firefox Aurora 1080p 60.04 1.00 60.04 1.14 60.02 1.18 > > OSX: > FPS UPms FPS UPms FPS UPms > Chrome 36 1080p 59.56 9.91 59.72 9.96 59.78 9.91 > Chrome 35 1080p 59.77 13.47 58.88 14.51 59.85 13.69 > Firefox Aurora 1080p N/A N/A 59.50 1.54 59.07 1.50 > Safari 6.1.4 1080p 20.07 34.94 N/A N/A N/A N/A > > Windows 8.1: > > FPS UPms FPS UPms FPS UPms > Chrome 36 1080p 51.22 11.53 59.53 7.16 59.23 7.23 > Firefox Aurora 1080p 57.14 9.55 56.89 9.41 > IE11 video not supported for upload > > Conclusion: Firefox has made good efforts in its Aurora build (scheduled to > be beta early September, and release early October) on OSX and Linux, these > now outperform Chrome by a substantial margin (by ~600%) and pushed upload > times into the range of 1-2ms. Chrome 36 has also made some gains on Chrome > 35 on OSX (by ~45%). Firefox has yet to implement their improvements on > Windows and has also got to deal with bugs in their h.264 support on Windows. > > Firefox issues: > > - The optimization from this ticket > https://bugzilla.mozilla.org/show_bug.cgi?id=814524 has not made it to the > Firefox Aurora windows build. Is this a new error? Should I open a ticket > for it? Or is this tracked by this ticket? > - The H.264 implementation of Firefox is buggy. It's quite jittery and > often crashes (I have not looked for a ticket for this yet, is this known?) This bug deal with the 2D(a.k.a CanvasRenderingContext2D) case, for WebGL case please go to bug 814524. By the way, I have no idea about the Windows problem. Maybe it defaults to ANGLE path, and ANGLE has something different there.

(In reply to Chiajung Hung [:chiajung] from comment #34) > By the way, I have no idea about the Windows problem. Maybe it defaults to > ANGLE path, and ANGLE has something different there. Windows is ANGLE unless we mess around with prefs. The only way to not use ANGLE on windows is if you go out of your way to disable it.

Comment on attachment 8444881 [details] [diff] [review] V4 Review of attachment 8444881 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/canvas/src/CanvasRenderingContext2D.cpp @@ +3318,5 @@ > + mozilla::gl::GLContext* gl = gfxPlatform::GetPlatform()->GetSkiaGLGlue()->GetGLContext(); > + > + HTMLVideoElement* video = &image.GetAsHTMLVideoElement(); > + if (!video) { > + return; I think we could fall back to original case, not just return directly. @@ +3325,5 @@ > + uint16_t readyState; > + if (NS_SUCCEEDED(video->GetReadyState(&readyState)) && > + readyState < nsIDOMHTMLMediaElement::HAVE_CURRENT_DATA) { > + //No frame inside, just return > + return; As same as previous comment. @@ +3331,5 @@ > + > + // If it doesn't have a principal, just bail > + nsCOMPtr<nsIPrincipal> principal = video->GetCurrentPrincipal(); > + if (!principal) { > + return; As same as previous comment @@ +3335,5 @@ > + return; > + } > + > + mozilla::layers::ImageContainer* container = video->GetImageContainer(); > + if (!container) { As same as previous comment @@ +3340,5 @@ > + return; > + } > + > + nsRefPtr<mozilla::layers::Image> srcImage = container->LockCurrentImage(); > + if (!srcImage) { As same as previous comment @@ +3355,5 @@ > + gl->fBindTexture(LOCAL_GL_TEXTURE_2D, mVideoTexture); > + > + bool dimensionsMatch = mCurrentVideoWidth == srcImage->GetSize().width && > + mCurrentVideoHeight == srcImage->GetSize().height; > + if (!dimensionsMatch) { You can move 3357 and 3358 as the if condition @@ +3363,5 @@ > + gl->fTexImage2D(LOCAL_GL_TEXTURE_2D, 0, LOCAL_GL_RGBA, srcImage->GetSize().width, srcImage->GetSize().height, 0, LOCAL_GL_RGBA, LOCAL_GL_UNSIGNED_BYTE, nullptr); > + gl->fTexParameteri(LOCAL_GL_TEXTURE_2D, LOCAL_GL_TEXTURE_WRAP_S, LOCAL_GL_CLAMP_TO_EDGE); > + gl->fTexParameteri(LOCAL_GL_TEXTURE_2D, LOCAL_GL_TEXTURE_WRAP_T, LOCAL_GL_CLAMP_TO_EDGE); > + gl->fTexParameteri(LOCAL_GL_TEXTURE_2D, LOCAL_GL_TEXTURE_MAG_FILTER, LOCAL_GL_LINEAR); > + gl->fTexParameteri(LOCAL_GL_TEXTURE_2D, LOCAL_GL_TEXTURE_MIN_FILTER, LOCAL_GL_LINEAR); I think we have the GL util to configure the texture, like the CreateTexture from GLBlitHelper.cpp. Or merge the GL related code inside BlitImageToTexture, just like BindAndUploadExternalTexture @@ +3371,5 @@ > + srcSurf = mTarget->CreateSourceSurfaceFromTexture(mVideoTexture, IntSize(mCurrentVideoWidth, mCurrentVideoHeight), SurfaceFormat::R8G8B8A8); > + imgSize.width = mCurrentVideoWidth; > + imgSize.height = mCurrentVideoHeight; > + } > + srcImage = nullptr; srcImage will be destroyed after this block. No need to clear srcImage. ::: gfx/2d/2D.h @@ +858,2 @@ > int32_t aStride, > SurfaceFormat aFormat) const = 0; Please add comment for this new API ::: gfx/2d/DrawTargetSkia.cpp @@ +594,5 @@ > +TemporaryRef<SourceSurface> > +DrawTargetSkia::CreateSourceSurfaceFromTexture(unsigned int aTexture, > + const IntSize& aSize, > + SurfaceFormat aFormat) const > +{ return nullptr if UsingSkiaGPU() is false

Rebased and slightly cleaned up. Fix a tiny bug in last version which make it unable to dynamic switch backend. Thanks to the flexibility of 2D API, this do not need changes in 2D API anymore.

Comment on attachment 8512362 [details] [diff] [review] V5 Review of attachment 8512362 [details] [diff] [review]: ----------------------------------------------------------------- Looks pretty good to me; see inline comments for minor observations. ::: dom/canvas/CanvasRenderingContext2D.cpp @@ +821,5 @@ > CanvasRenderingContext2D::CanvasRenderingContext2D() > : mRenderingMode(RenderingMode::OpenGLBackendMode) > + , mVideoTexture(0) > + , mCurrentVideoWidth(0) > + , mCurrentVideoHeight(0) USE_SKIA_GPU @@ +854,5 @@ > NS_IF_RELEASE(sErrorTarget); > } > + if (mVideoTexture) { > + MOZ_ASSERT(gfxPlatform::GetPlatform()->GetSkiaGLGlue(), "null SkiaGLGlue"); > + gfxPlatform::GetPlatform()->GetSkiaGLGlue()->GetGLContext()->fDeleteTextures(1, &mVideoTexture); USE_SKIA_GPU @@ +1059,5 @@ > > + if (aRenderingMode != RenderingMode::OpenGLBackendMode && > + mRenderingMode == RenderingMode::OpenGLBackendMode) { > + if (mVideoTexture) { > + gfxPlatform::GetPlatform()->GetSkiaGLGlue()->GetGLContext()->fDeleteTextures(1, &mVideoTexture); USE_SKIA_GPU @@ +3975,5 @@ > + if (!dimensionsMatch) { > + // we need to allocation > + mCurrentVideoWidth = srcImage->GetSize().width; > + mCurrentVideoHeight = srcImage->GetSize().height; > + gl->fTexImage2D(LOCAL_GL_TEXTURE_2D, 0, LOCAL_GL_RGBA, srcImage->GetSize().width, srcImage->GetSize().height, 0, LOCAL_GL_RGBA, LOCAL_GL_UNSIGNED_BYTE, nullptr); Are video textures always R8G8B8A8? ::: dom/canvas/CanvasRenderingContext2D.h @@ +743,5 @@ > > + // Texture informations for fast video rendering > + unsigned int mVideoTexture; > + unsigned int mCurrentVideoWidth; > + unsigned int mCurrentVideoHeight; Should this just be an nsIntSize? ::: gfx/2d/SourceSurfaceSkia.h @@ +43,5 @@ > > + bool InitFromTexture(GrContext* aGr, > + unsigned int aTexture, > + const IntSize &aSize, > + SurfaceFormat aFormat); I feel like instead of passing in the GrContext, we should pass in a DrawTargetSkia owner instead, like we do with InitFromCanvas. At the very least, I think we'll need to keep track of the GrContext passed in here to ensure that we do the right thing if we try to draw a SourceSurfaceSkia which is wrapping a texture on one GrContext to a DrawTargetSkia which uses a different GrContext, right?

Comment on attachment 8512362 [details] [diff] [review] V5 Review of attachment 8512362 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the review. ::: dom/canvas/CanvasRenderingContext2D.cpp @@ +821,5 @@ > CanvasRenderingContext2D::CanvasRenderingContext2D() > : mRenderingMode(RenderingMode::OpenGLBackendMode) > + , mVideoTexture(0) > + , mCurrentVideoWidth(0) > + , mCurrentVideoHeight(0) OK. @@ +854,5 @@ > NS_IF_RELEASE(sErrorTarget); > } > + if (mVideoTexture) { > + MOZ_ASSERT(gfxPlatform::GetPlatform()->GetSkiaGLGlue(), "null SkiaGLGlue"); > + gfxPlatform::GetPlatform()->GetSkiaGLGlue()->GetGLContext()->fDeleteTextures(1, &mVideoTexture); OK @@ +1059,5 @@ > > + if (aRenderingMode != RenderingMode::OpenGLBackendMode && > + mRenderingMode == RenderingMode::OpenGLBackendMode) { > + if (mVideoTexture) { > + gfxPlatform::GetPlatform()->GetSkiaGLGlue()->GetGLContext()->fDeleteTextures(1, &mVideoTexture); OK. @@ +3975,5 @@ > + if (!dimensionsMatch) { > + // we need to allocation > + mCurrentVideoWidth = srcImage->GetSize().width; > + mCurrentVideoHeight = srcImage->GetSize().height; > + gl->fTexImage2D(LOCAL_GL_TEXTURE_2D, 0, LOCAL_GL_RGBA, srcImage->GetSize().width, srcImage->GetSize().height, 0, LOCAL_GL_RGBA, LOCAL_GL_UNSIGNED_BYTE, nullptr); We originally always use RGB565, see: http://dxr.mozilla.org/mozilla-central/source/gfx/layers/GrallocImages.cpp#418 But RGB565 may produce slightly worse quality compare to YUV, so I choose RGBA8888 here, maybe I can use 565 and enable dithering? BTW, I think the spec does not specify how should we handle YUV frames. ::: dom/canvas/CanvasRenderingContext2D.h @@ +743,5 @@ > > + // Texture informations for fast video rendering > + unsigned int mVideoTexture; > + unsigned int mCurrentVideoWidth; > + unsigned int mCurrentVideoHeight; OK. ::: gfx/2d/SourceSurfaceSkia.h @@ +43,5 @@ > > + bool InitFromTexture(GrContext* aGr, > + unsigned int aTexture, > + const IntSize &aSize, > + SurfaceFormat aFormat); OK, I will try to pass in the owner DrawTarget instead. AFAICT, The only important thing should be make sure the texture is in fact created by the underlying context of the GrContext. IIRC we use shared/global context for all CanvasRenderingContext2D's DrawTargetSkia, so I didn't take care about that. :) Since there is no way to check whether the texture passed in is created from the GrContext/GLContext, the user of this API should be very careful. I will leave some comment about that here.

Fix review comment, carry r+

(In reply to Chiajung Hung [:chiajung] from comment #39) > We originally always use RGB565, see: > http://dxr.mozilla.org/mozilla-central/source/gfx/layers/GrallocImages. > cpp#418 > > But RGB565 may produce slightly worse quality compare to YUV, so I choose > RGBA8888 here, maybe I can use 565 and enable dithering? > > BTW, I think the spec does not specify how should we handle YUV frames. I guess that makes sense. It might be worth adding a comment here then stating that we will always get a frame in RGBA8888 instead of RGB565. > AFAICT, The only important thing should be make sure the texture is in fact > created by the underlying context of the GrContext. IIRC we use > shared/global context for all CanvasRenderingContext2D's DrawTargetSkia, so > I didn't take care about that. :) > > Since there is no way to check whether the texture passed in is created from > the GrContext/GLContext, the user of this API should be very careful. I will > leave some comment about that here. Right, but the API from DrawTargetSkia doesn't guarantee that we use the same GrContext everywhere, we just happen to do so for Gecko :)

Fix debug build fail and add some comment, carry r+. (In reply to George Wright (:gw280) from comment #41) > (In reply to Chiajung Hung [:chiajung] from comment #39) > > We originally always use RGB565, see: > > http://dxr.mozilla.org/mozilla-central/source/gfx/layers/GrallocImages. > > cpp#418 > > > > But RGB565 may produce slightly worse quality compare to YUV, so I choose > > RGBA8888 here, maybe I can use 565 and enable dithering? > > > > BTW, I think the spec does not specify how should we handle YUV frames. > > I guess that makes sense. It might be worth adding a comment here then > stating that we will always get a frame in RGBA8888 instead of RGB565. I decided to use RGB565 :p As the quality enhance would hard to notice, and RGB565 use half of memory. We can change to RGB8 or RGBA8 if needed in the future. > > > AFAICT, The only important thing should be make sure the texture is in fact > > created by the underlying context of the GrContext. IIRC we use > > shared/global context for all CanvasRenderingContext2D's DrawTargetSkia, so > > I didn't take care about that. :) > > > > Since there is no way to check whether the texture passed in is created from > > the GrContext/GLContext, the user of this API should be very careful. I will > > leave some comment about that here. > > Right, but the API from DrawTargetSkia doesn't guarantee that we use the > same GrContext everywhere, we just happen to do so for Gecko :) Agree. In fact, the wrapping SourceSurfaceSkia in this case is not needed at all (We can simply wrap and draw in Canvas2D::DrawImage directly). I wrote the patch this way to hide most backend specific part from CanvasRenderingContext2D. Though some GL code remains :(

Comment on attachment 8513153 [details] [diff] [review] V6.1 Review of attachment 8513153 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/2d/SourceSurfaceSkia.cpp @@ +96,5 @@ > + unsigned int aTexture, > + const IntSize &aSize, > + SurfaceFormat aFormat) > +{ > + MOZ_ASSERT(aOwner, "null GrContext"); You never set mDrawTarget?

I was thinking mDrawTarget in this case is Snapshot related, and may change the semantics there: var pixels = ctx.getImageData(); // snapshot created and cached in DrawTargetSkia ctx.drawImage(video); // if I set mDrawTarget, the DrawTargetSkia will think the snapshot is gone because the temp video surface is out of scope However, I found it is OK, as we will clear the cache as soon as we call to DrawSurface. I will re-add that line later.

Fixes mochitest fails. Since SkiaGL canvas default on B2G, only B2G tested. Try tickets: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=f220c92d8f4c

sorry had to back this out for test failures like https://treeherder.mozilla.org/ui/logviewer.html#?job_id=3681868&repo=mozilla-inbound

OK, I'll look into it. Is it Android only? I think I had fixed exactly same thing on B2G before.

yeah confirmed, it was a Android only test failure

Fix Android mochitest, try ticket: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=0edc02e80013