Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1111653004:
Replace SW YUV conversion with higher quality Shader+FBO in pepper video path. (Closed)
|
Created:
5 years, 7 months ago by CodeByThePound Modified:
5 years, 7 months ago CC:
chromium-reviews, mkwst+moarreviews-renderer_chromium.org, darin-cc_chromium.org, jam, mlamouri+watch-content_chromium.org, sandersd (OOO until July 31), elijahtaylor1 Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionReplace SW YUV conversion with higher quality Shader+FBO in pepper video path.
Improve quality of software rendering path by replacing call to SkCanvasVideoRenderer::ConvertVideoFrameToRGB in video_decoder_shim with a shader and render-to-texture.
Questions about this CL:
* gpu::gles2::GLESInterface
Would it be correct to send a base::WeakPtr<GLESInterface> to the YUVConv
class constructor to use for everything in this instance of the class?
ATM, it just sends a GLESInterface* to methods that need it (which is
probably wrong). It was not clear to me whether the GLESInterface * could
change between the ::Initialize() method
* OpenGL state
YUVConv::convert() changes texture bindings and viewport. I currently read
the old viewport so that I can restore it - is this necessary? I don't save
former texture bindings either - is this necessary?
* OpenGL 'getters'
When compiled debug, why do all output variables to GL functions need to
contain either 0 or -1 before calling the function? In my case GetIntegerv
and GetShaderiv.
* Errors
It is possible that shader compile could fail. If it fails, an error string
can be retrieved. What should I do in this case?
Same with opengl errors. I thought it would be good to check for them in
debug builds, but what do I do if I catch one?
* Texture formats
I need a single-component texture format for uploading the Y,U,V components.
On desktop I would use GL_RED, but unextended GLES does not have GL_RED, you
have to use GL_LUMINANCE. The current code uses GL_RED, and it works, but do
I need to check for GLES extension for GL_RED or can I assume it is there?
* Texture sizes
Even with the description in video_frame.h the distinction between coded_size
visible_size and natural_size was not entirely clear to me. It seemed the
output texture size in VideoDecoderShim::OnOutputComplete followed "coded_size"
so that is what I used as well. It was my intention to always decode the
entire image to the user's buffer, not some subregion - so is my code correct?
* FBO's
I see that DrawBuffers is DrawBuffersEXT, and that the default GLES
implementation does not support it. Is it not necessary in this environment
to call DrawBuffers?
* ::SendPictures
If there is a format error yuv_converter will return false. How can an error
be handled here?
* testing
I have tested with the video_decoder example program (using VPx) and our
application which uses h264. I believe both use I420 format. Do you have an
easy way to test other formats? Is this class part of a unit test?
BUG=483183, 477737
R=bbudge@chromium.org
TEST=Run pepper video_decoder example in ppapi/examples/video_decode
Committed: https://crrev.com/f5e606219178026623ee38907d14bff14cb83c7b
Cr-Commit-Position: refs/heads/master@{#329942}
Patch Set 1 #Patch Set 2 : Updates based on comments. #Patch Set 3 : Hopefully final changes. #Patch Set 4 : updates to failure path in CreateShader #
Total comments: 9
Patch Set 5 : Fix coding style issues. #
Total comments: 10
Patch Set 6 : Fixed a Windows build warning. #Patch Set 7 : Added InvalidateGrContext calls for Skia State. #
Total comments: 42
Patch Set 8 : Fixed nits. Added support for all YUV formats that SW path supported. #Patch Set 9 : Removed unwanted comments. #Patch Set 10 : Added missing period. #
Total comments: 23
Patch Set 11 : Further refinements. #
Messages
Total messages: 55 (15 generated)
bbudge@chromium.org changed reviewers: + bbudge@chromium.org - bbudge@google.com
High level comments: 1) Read chromium coding style, based on Google coding style. 2) Is there a reason the converter class is declared as a child class? It might be simpler to move it out. Answers to (some of) your questions: * gpu::gles2::GLESInterface Would it be correct to send a base::WeakPtr<GLESInterface> to the YUVConv class constructor to use for everything in this instance of the class? ATM, it just sends a GLESInterface* to methods that need it (which is probably wrong). It was not clear to me whether the GLESInterface * could change between the ::Initialize() method > I don't think you have to worry about the lifetime of GLES2Interface. * OpenGL state YUVConv::convert() changes texture bindings and viewport. I currently read the old viewport so that I can restore it - is this necessary? I don't save former texture bindings either - is this necessary? > I think you just have to worry about not leaking textures and resources. * Errors It is possible that shader compile could fail. If it fails, an error string can be retrieved. What should I do in this case? Same with opengl errors. I thought it would be good to check for them in debug builds, but what do I do if I catch one? > You should flow errors into the existing Pepper error reporting, if the error is something the plugin needs to know about. Hopefully this doesn't happen much in practice, or at least no more than the existing software implementation. In the case you mention, the shader should be compiled at initialization time, and any error is reported as initialization failure. * Texture sizes Even with the description in video_frame.h the distinction between coded_size visible_size and natural_size was not entirely clear to me. It seemed the output texture size in VideoDecoderShim::OnOutputComplete followed "coded_size" so that is what I used as well. It was my intention to always decode the entire image to the user's buffer, not some subregion - so is my code correct? > I think that's right. * ::SendPictures If there is a format error yuv_converter will return false. How can an error be handled here? > I don't think we handle that case right now. The assumption is that the decoder will always return valid pictures, or report an error earlier. * testing I have tested with the video_decoder example program (using VPx) and our application which uses h264. I believe both use I420 format. Do you have an easy way to test other formats? Is this class part of a unit test? > Unfortunately we don't have good unit tests for the guts of this. I would hack the example plugin.
codebythepound@gmail.com changed reviewers: + sandersd@chromium.org
Based on comments, I made the following changes: Moved class outside of VideoDecoderShim. Fixed some formatting, and some comments. Pass GLESContext pointer to Startup() and cache it. Check if texture_rg is supported, use GL_LUMINANCE if not. Changed GL_RED to GL_RED_EXT Removed call to DrawBuffersEXT as it is not necessary. Modified convert code to color texture red if YUV format was not supported.
Now passes cc_blink::ContextProviderWebContext to constructor. Removed Shutdown method and now shuts things down in destructor. Textual errors are logged using the standard mechanism. GL errors seem to be handled automatically in debug mode, so removed GL error handling. Changed group marker to YUVConvContext to match PPAPIContext.
codebythepound@gmail.com changed required reviewers: + bbudge@chromium.org
Please review. Thanks.
A few comments. Before I go any deeper, I suggest running 'git cl format' on your branch to get the formatting closer to Chromium standard. https://codereview.chromium.org/1111653004/diff/60001/content/renderer/pepper... File content/renderer/pepper/video_decoder_shim.cc (right): https://codereview.chromium.org/1111653004/diff/60001/content/renderer/pepper... content/renderer/pepper/video_decoder_shim.cc:36: public: chromium indent of public/private is 1 space, then 1 more space for decls. Just use 'git cl format'. https://codereview.chromium.org/1111653004/diff/60001/content/renderer/pepper... content/renderer/pepper/video_decoder_shim.cc:37: YUVConv( const scoped_refptr<cc_blink::ContextProviderWebContext> & ); Chromium coding style: no space after '(', '>', and before ')'. https://codereview.chromium.org/1111653004/diff/60001/content/renderer/pepper... content/renderer/pepper/video_decoder_shim.cc:41: GLuint texOut ); Style: Chrome uses texture_out style rather than camel case. https://codereview.chromium.org/1111653004/diff/60001/content/renderer/pepper... content/renderer/pepper/video_decoder_shim.cc:45: GLuint CompileShader( const char * name, GLuint type, const char * code ); s/char * name/char* name https://codereview.chromium.org/1111653004/diff/60001/content/renderer/pepper... content/renderer/pepper/video_decoder_shim.cc:51: private: duplicate private https://codereview.chromium.org/1111653004/diff/60001/content/renderer/pepper... content/renderer/pepper/video_decoder_shim.cc:79: const scoped_refptr<cc_blink::ContextProviderWebContext> & ctx_p ) s/ctx_p/context_provider You can use 'git cl format' to clang-format just your patch. This can take out a lot of the tedium of conforming to chromium style. https://codereview.chromium.org/1111653004/diff/60001/content/renderer/pepper... content/renderer/pepper/video_decoder_shim.cc:81: , gl_( context_provider_->ContextGL() ) Chromium style puts the commas on the previous line. https://codereview.chromium.org/1111653004/diff/60001/content/renderer/pepper... File content/renderer/pepper/video_decoder_shim.h (right): https://codereview.chromium.org/1111653004/diff/60001/content/renderer/pepper... content/renderer/pepper/video_decoder_shim.h:42: class YUVConv; 'Conv' is a little unclear: YUVConverter seems better for the class name. https://codereview.chromium.org/1111653004/diff/60001/content/renderer/pepper... content/renderer/pepper/video_decoder_shim.h:122: // the YUV Converter renderer comment doesn't add much - you could delete it.
Sorry about the coding style issues. They should be fixed now. Ran git-cl format and changed all camelCase to unix_hacker_style.
bbudge@chromium.org changed reviewers: + piman@chromium.org - sandersd@chromium.org
On 2015/05/01 21:32:44, CodeByThePound wrote: > Sorry about the coding style issues. They should be fixed now. Ran git-cl > format and changed all camelCase to unix_hacker_style. +piman for general GL approach and correctness. Feel free to reassign as appropriate.
The CQ bit was checked by bbudge@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1111653004/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The approach seems reasonable overall. The main thing is the context sharing wrt Skia (which was admittedly true before but much more limited). https://codereview.chromium.org/1111653004/diff/80001/content/renderer/pepper... File content/renderer/pepper/video_decoder_shim.cc (right): https://codereview.chromium.org/1111653004/diff/80001/content/renderer/pepper... content/renderer/pepper/video_decoder_shim.cc:154: gl_->GetShaderiv(shader, GL_COMPILE_STATUS, &status); I wouldn't worry about this, at least in release mode. The shader is valid, it should be valid on all GPUs. I would skip the round trip. https://codereview.chromium.org/1111653004/diff/80001/content/renderer/pepper... content/renderer/pepper/video_decoder_shim.cc:186: gl_->GetProgramiv(program, GL_LINK_STATUS, &status); Same here. https://codereview.chromium.org/1111653004/diff/80001/content/renderer/pepper... content/renderer/pepper/video_decoder_shim.cc:206: if (loc != -1) { I think this should just be a DCHECK. https://codereview.chromium.org/1111653004/diff/80001/content/renderer/pepper... content/renderer/pepper/video_decoder_shim.cc:350: // set group marker nit: most of these comments are redundant. Please remove unless they add additional informations / explain non-obvious things. https://codereview.chromium.org/1111653004/diff/80001/content/renderer/pepper... content/renderer/pepper/video_decoder_shim.cc:530: // MMM - is this necessary? Well... the SharedMainThreadContextProvider is also used by Skia (for accelerated 2D canvas), so modifying state would probably make Skia's cache invalid. This is true of the viewport, but also buffer bindings, texture bindings, current program, etc. So these would need to be saved/restored too. The problem is that a lot of state isn't cached on the client side, so this would require an expensive round trip to the GPU process, and defeat the performance gains. One solution could be to modify the state and invalidate Skia's cache, via GrContext::resetContext. It would have some performance overhead if Skia is used on the same page, but would be virtually null if not. Maybe the best way for that is to extend ContextProvider to add an InvalidateGrContext(), to make sure we don't unnecessarily create a GrContext if none exists (ContextProvider::GrContext() will lazily create it). The other solution would be to create a new context for the VideoDecoderShim instead of using the shared one. This is safe, but would add some fair amount of memory overhead (regardless of the page usage of 2D canvas). I tend to favor the former.
https://codereview.chromium.org/1111653004/diff/80001/content/renderer/pepper... File content/renderer/pepper/video_decoder_shim.cc (right): https://codereview.chromium.org/1111653004/diff/80001/content/renderer/pepper... content/renderer/pepper/video_decoder_shim.cc:154: gl_->GetShaderiv(shader, GL_COMPILE_STATUS, &status); On 2015/05/05 02:21:55, piman (Very slow to review) wrote: > I wouldn't worry about this, at least in release mode. The shader is valid, it > should be valid on all GPUs. I would skip the round trip. Acknowledged. https://codereview.chromium.org/1111653004/diff/80001/content/renderer/pepper... content/renderer/pepper/video_decoder_shim.cc:186: gl_->GetProgramiv(program, GL_LINK_STATUS, &status); On 2015/05/05 02:21:55, piman (Very slow to review) wrote: > Same here. Acknowledged. https://codereview.chromium.org/1111653004/diff/80001/content/renderer/pepper... content/renderer/pepper/video_decoder_shim.cc:206: if (loc != -1) { On 2015/05/05 02:21:55, piman (Very slow to review) wrote: > I think this should just be a DCHECK. Acknowledged. https://codereview.chromium.org/1111653004/diff/80001/content/renderer/pepper... content/renderer/pepper/video_decoder_shim.cc:350: // set group marker On 2015/05/05 02:21:55, piman (Very slow to review) wrote: > nit: most of these comments are redundant. Please remove unless they add > additional informations / explain non-obvious things. Acknowledged. https://codereview.chromium.org/1111653004/diff/80001/content/renderer/pepper... content/renderer/pepper/video_decoder_shim.cc:530: // MMM - is this necessary? Do you want me to make this modification as part of this CL or a separate CL?
On 2015/05/05 22:00:06, CodeByThePound wrote: > https://codereview.chromium.org/1111653004/diff/80001/content/renderer/pepper... > File content/renderer/pepper/video_decoder_shim.cc (right): > > https://codereview.chromium.org/1111653004/diff/80001/content/renderer/pepper... > content/renderer/pepper/video_decoder_shim.cc:154: gl_->GetShaderiv(shader, > GL_COMPILE_STATUS, &status); > On 2015/05/05 02:21:55, piman (Very slow to review) wrote: > > I wouldn't worry about this, at least in release mode. The shader is valid, it > > should be valid on all GPUs. I would skip the round trip. > > Acknowledged. > > https://codereview.chromium.org/1111653004/diff/80001/content/renderer/pepper... > content/renderer/pepper/video_decoder_shim.cc:186: gl_->GetProgramiv(program, > GL_LINK_STATUS, &status); > On 2015/05/05 02:21:55, piman (Very slow to review) wrote: > > Same here. > > Acknowledged. > > https://codereview.chromium.org/1111653004/diff/80001/content/renderer/pepper... > content/renderer/pepper/video_decoder_shim.cc:206: if (loc != -1) { > On 2015/05/05 02:21:55, piman (Very slow to review) wrote: > > I think this should just be a DCHECK. > > Acknowledged. > > https://codereview.chromium.org/1111653004/diff/80001/content/renderer/pepper... > content/renderer/pepper/video_decoder_shim.cc:350: // set group marker > On 2015/05/05 02:21:55, piman (Very slow to review) wrote: > > nit: most of these comments are redundant. Please remove unless they add > > additional informations / explain non-obvious things. > > Acknowledged. > > https://codereview.chromium.org/1111653004/diff/80001/content/renderer/pepper... > content/renderer/pepper/video_decoder_shim.cc:530: // MMM - is this necessary? > Do you want me to make this modification as part of this CL or a separate CL? It has to be this CL I think - this CL regresses in that it modifies more state from the shared context, I don't think we want to land that.
> > Do you want me to make this modification as part of this CL or a separate CL? > > It has to be this CL I think - this CL regresses in that it modifies more state > from the shared context, I don't think we want to land that. I guess I wasn't clear - I meant do you want me to land a separate CL before this one that adds the new method, and then use it in this CL. But I'm happy to make it to this CL.
On Tue, May 5, 2015 at 4:23 PM, <CodeByThePound@gmail.com> wrote: > > Do you want me to make this modification as part of this CL or a separate >> > CL? > > It has to be this CL I think - this CL regresses in that it modifies more >> > state > >> from the shared context, I don't think we want to land that. >> > > I guess I wasn't clear - I meant do you want me to land a separate CL > before > this one that adds the new method, and then use it in this CL. But I'm > happy to > make it to this CL. > Ah ok, fair enough, that sounds like a good idea. > > > https://codereview.chromium.org/1111653004/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Did you have something as simple as a non-virtual method like this in mind (in
context_provider.cc):
#include "cc/output/context_provider.h"
#include "third_party/skia/include/gpu/GrContext.h"
#include <limits>
namespace cc {
ContextProvider::Capabilities::Capabilities()
: max_transfer_buffer_usage_bytes(std::numeric_limits<size_t>::max()) {}
void ContextProvider::InvalidateGrContext() {
if(GrContext())
GrContext()->resetContext();
}
Or were you thinking it should be virtual/pure virtual where all the derived
classes of context_provider should implement it?
Also resetContext() exposes some resetbits so that the entire context doesn't
need to be reset each time. Should I expose those as well?
On 2015/05/06 18:05:43, CodeByThePound wrote:
> Did you have something as simple as a non-virtual method like this in mind (in
> context_provider.cc):
>
> #include "cc/output/context_provider.h"
> #include "third_party/skia/include/gpu/GrContext.h"
>
> #include <limits>
>
> namespace cc {
>
> ContextProvider::Capabilities::Capabilities()
> : max_transfer_buffer_usage_bytes(std::numeric_limits<size_t>::max()) {}
>
> void ContextProvider::InvalidateGrContext() {
> if(GrContext())
> GrContext()->resetContext();
> }
This wouldn't do the right thing, because GrContext() would lazily create the
GrContext, and essentially always return true. The idea of InvalidateGrContext
is that it wouldn't try to create one, just reset it if it exists.
>
> Or were you thinking it should be virtual/pure virtual where all the derived
> classes of context_provider should implement it?
I think you need that.
> Also resetContext() exposes some resetbits so that the entire context doesn't
> need to be reset each time. Should I expose those as well?
That seems reasonable, if we can use the flags to be more explicit about which
state was clobbered.
Created https://codereview.chromium.org/1131723002/ for the InvalidateGrContext() method.
Added InvalidateGrContext() calls to Initialize and Convert.
https://codereview.chromium.org/1111653004/diff/120001/content/renderer/peppe... File content/renderer/pepper/video_decoder_shim.cc (right): https://codereview.chromium.org/1111653004/diff/120001/content/renderer/peppe... content/renderer/pepper/video_decoder_shim.cc:35: // nit: remove blank comment line https://codereview.chromium.org/1111653004/diff/120001/content/renderer/peppe... content/renderer/pepper/video_decoder_shim.cc:36: // YUV->RGB converter class using a shader and FBO nit: end with a . https://codereview.chromium.org/1111653004/diff/120001/content/renderer/peppe... content/renderer/pepper/video_decoder_shim.cc:37: // nit: remove blank comment line https://codereview.chromium.org/1111653004/diff/120001/content/renderer/peppe... content/renderer/pepper/video_decoder_shim.cc:104: kProgram_GrGLBackendState) { I don't think it's useful to have it as a field since it doesn't change. Make it a global constant? static const uint32_t kGrInvalidateState = RenderTarget_GrGLBackendState | kTextureBinding_GrGLBackendState | kView_GrGLBackendState | kVertex_GrGLBackendState | kProgram_GrGLBackendState; https://codereview.chromium.org/1111653004/diff/120001/content/renderer/peppe... content/renderer/pepper/video_decoder_shim.cc:108: // delete textures nit: remove comment https://codereview.chromium.org/1111653004/diff/120001/content/renderer/peppe... content/renderer/pepper/video_decoder_shim.cc:118: // delete framebuffer nit: remove comment https://codereview.chromium.org/1111653004/diff/120001/content/renderer/peppe... content/renderer/pepper/video_decoder_shim.cc:122: // delete vertex buffer nit: remove comment https://codereview.chromium.org/1111653004/diff/120001/content/renderer/peppe... content/renderer/pepper/video_decoder_shim.cc:126: // delete program nit: remove comment https://codereview.chromium.org/1111653004/diff/120001/content/renderer/peppe... content/renderer/pepper/video_decoder_shim.cc:134: // create texture nit: remove comment https://codereview.chromium.org/1111653004/diff/120001/content/renderer/peppe... content/renderer/pepper/video_decoder_shim.cc:142: // set params nit: remove comment https://codereview.chromium.org/1111653004/diff/120001/content/renderer/peppe... content/renderer/pepper/video_decoder_shim.cc:148: // unbind nit: remove comment https://codereview.chromium.org/1111653004/diff/120001/content/renderer/peppe... content/renderer/pepper/video_decoder_shim.cc:171: // The maxLength includes the NULL character nit: max_length https://codereview.chromium.org/1111653004/diff/120001/content/renderer/peppe... content/renderer/pepper/video_decoder_shim.cc:191: // bind known vertex position nit: remove comment https://codereview.chromium.org/1111653004/diff/120001/content/renderer/peppe... content/renderer/pepper/video_decoder_shim.cc:205: // The maxLength includes the NULL character nit: max_length https://codereview.chromium.org/1111653004/diff/120001/content/renderer/peppe... content/renderer/pepper/video_decoder_shim.cc:242: // convert vertex positions to texcoords, clamp x nit: remove comment https://codereview.chromium.org/1111653004/diff/120001/content/renderer/peppe... content/renderer/pepper/video_decoder_shim.cc:279: // nit: remove comment https://codereview.chromium.org/1111653004/diff/120001/content/renderer/peppe... content/renderer/pepper/video_decoder_shim.cc:295: // nit: remove comment https://codereview.chromium.org/1111653004/diff/120001/content/renderer/peppe... content/renderer/pepper/video_decoder_shim.cc:299: // delete shaders since they are no longer needed nit: remove comment https://codereview.chromium.org/1111653004/diff/120001/content/renderer/peppe... content/renderer/pepper/video_decoder_shim.cc:311: // nit: remove comment https://codereview.chromium.org/1111653004/diff/120001/content/renderer/peppe... content/renderer/pepper/video_decoder_shim.cc:316: // set default clamp width nit: remove comment https://codereview.chromium.org/1111653004/diff/120001/content/renderer/peppe... content/renderer/pepper/video_decoder_shim.cc:319: // store clamp width loc for later nit: remove comment https://codereview.chromium.org/1111653004/diff/120001/content/renderer/peppe... content/renderer/pepper/video_decoder_shim.cc:331: // nit: remove blank comment line https://codereview.chromium.org/1111653004/diff/120001/content/renderer/peppe... content/renderer/pepper/video_decoder_shim.cc:334: // nit: remove blank comment line https://codereview.chromium.org/1111653004/diff/120001/content/renderer/peppe... content/renderer/pepper/video_decoder_shim.cc:348: // Create some default textures to hold Y,U,V values nit: remove comment https://codereview.chromium.org/1111653004/diff/120001/content/renderer/peppe... content/renderer/pepper/video_decoder_shim.cc:376: // make sure we support this format nit: remove comment https://codereview.chromium.org/1111653004/diff/120001/content/renderer/peppe... content/renderer/pepper/video_decoder_shim.cc:388: // rest are not supported.. nit: remove comment https://codereview.chromium.org/1111653004/diff/120001/content/renderer/peppe... content/renderer/pepper/video_decoder_shim.cc:407: // resize textures if necessary nit: remove comment https://codereview.chromium.org/1111653004/diff/120001/content/renderer/peppe... content/renderer/pepper/video_decoder_shim.cc:410: // then, clamp later You can keep this comment, but it should be a full sentence, starting with an uppercase letter and ending with a period. Maybe explain the full clamping story here, rather than distributed all in the code. https://codereview.chromium.org/1111653004/diff/120001/content/renderer/peppe... content/renderer/pepper/video_decoder_shim.cc:417: if ((ystride != ywidth) || (clamp_width_ != ywidth)) { I'm not sure I understand the need for this outer if. The if on the line below makes sure the code does nothing if clamp_width_ == ywidth. Remove? https://codereview.chromium.org/1111653004/diff/120001/content/renderer/peppe... content/renderer/pepper/video_decoder_shim.cc:419: // clamp width to avoid sampling padding pixels nit: remove comment (assuming you explained above). https://codereview.chromium.org/1111653004/diff/120001/content/renderer/peppe... content/renderer/pepper/video_decoder_shim.cc:423: // clamp to 1/2 pixel inside to avoid bilinear sampling errors nit: remove comment (assuming you explained above). https://codereview.chromium.org/1111653004/diff/120001/content/renderer/peppe... content/renderer/pepper/video_decoder_shim.cc:432: // nit: remove comment https://codereview.chromium.org/1111653004/diff/120001/content/renderer/peppe... content/renderer/pepper/video_decoder_shim.cc:453: // nit: remove comment https://codereview.chromium.org/1111653004/diff/120001/content/renderer/peppe... content/renderer/pepper/video_decoder_shim.cc:476: // If the format is not supported by the fast path, we should keep the old path - convert in software directly into the BGRA texture. FYI, the reason 2x2 doesn't work is likely because of the default GL_UNPACK_ALIGNMENT which is 4. https://codereview.chromium.org/1111653004/diff/120001/content/renderer/peppe... content/renderer/pepper/video_decoder_shim.cc:508: // will slow this method down so check in debug mode nit: end with a . https://codereview.chromium.org/1111653004/diff/120001/content/renderer/peppe... content/renderer/pepper/video_decoder_shim.cc:535: // active texture is still 2 nit: remove comment https://codereview.chromium.org/1111653004/diff/120001/content/renderer/peppe... content/renderer/pepper/video_decoder_shim.cc:853: // initialize yuv converter nit: remove comment https://codereview.chromium.org/1111653004/diff/120001/content/renderer/peppe... content/renderer/pepper/video_decoder_shim.cc:1035: // run the yuv conversion renderer nit: remove comment https://codereview.chromium.org/1111653004/diff/120001/content/renderer/peppe... File content/renderer/pepper/video_decoder_shim.h (right): https://codereview.chromium.org/1111653004/diff/120001/content/renderer/peppe... content/renderer/pepper/video_decoder_shim.h:42: class YUVConverter; nit: make it an inner class of VideoDecoderShim https://codereview.chromium.org/1111653004/diff/120001/content/renderer/peppe... content/renderer/pepper/video_decoder_shim.h:113: // Queue of decoded frames that await rgb->yuv conversion nit: end with a .
I'll remove all the requested comments and add the periods. > https://codereview.chromium.org/1111653004/diff/120001/content/renderer/peppe... > content/renderer/pepper/video_decoder_shim.cc:104: kProgram_GrGLBackendState) { > I don't think it's useful to have it as a field since it doesn't change. Make it > a global constant? > static const uint32_t kGrInvalidateState = RenderTarget_GrGLBackendState | > kTextureBinding_GrGLBackendState | > kView_GrGLBackendState | > kVertex_GrGLBackendState | > kProgram_GrGLBackendState; It seems like it should be a class member since its only used in this class. It is already const, I could make it static const or move it out of the class if that's what you want. > https://codereview.chromium.org/1111653004/diff/120001/content/renderer/peppe... > content/renderer/pepper/video_decoder_shim.cc:417: if ((ystride != ywidth) || > (clamp_width_ != ywidth)) { > I'm not sure I understand the need for this outer if. The if on the line below > makes sure the code does nothing if clamp_width_ == ywidth. > > Remove? I'll refactor this code a bit. > https://codereview.chromium.org/1111653004/diff/120001/content/renderer/peppe... > content/renderer/pepper/video_decoder_shim.cc:476: // > If the format is not supported by the fast path, we should keep the old path - > convert in software directly into the BGRA texture. I discussed this with Bill before I started and his opinion was it was OK for this path to just support planar 420, 422, and 444 - and that a fallback was not necessary. Do you want me to add the fallback anyway? > https://codereview.chromium.org/1111653004/diff/120001/content/renderer/peppe... > content/renderer/pepper/video_decoder_shim.h:42: class YUVConverter; > nit: make it an inner class of VideoDecoderShim Bill asked that I move it out of VideoDecoderShim class in my first CL. Should I put it back? > > FYI, the reason 2x2 doesn't work is likely because of the default > GL_UNPACK_ALIGNMENT which is 4. You are probably right. I'll set the unpack alignment to be 1 for this section, since its possible some video formats could be odd widths.
On Mon, May 11, 2015 at 10:47 PM, <CodeByThePound@gmail.com> wrote: > I'll remove all the requested comments and add the periods. > > > > https://codereview.chromium.org/1111653004/diff/120001/content/renderer/peppe... > >> content/renderer/pepper/video_decoder_shim.cc:104: >> kProgram_GrGLBackendState) >> > { > >> I don't think it's useful to have it as a field since it doesn't change. >> Make >> > it > >> a global constant? >> static const uint32_t kGrInvalidateState = RenderTarget_GrGLBackendState | >> kTextureBinding_GrGLBackendState | >> kView_GrGLBackendState | >> kVertex_GrGLBackendState | >> kProgram_GrGLBackendState; >> > > It seems like it should be a class member since its only used in this > class. It > is already const, I could make it static const or move it out of the class > if > that's what you want. I guess you can make it a static class member, but I wouldn't even bother and just leave it as a global static const - that's a common pattern. > > https://codereview.chromium.org/1111653004/diff/120001/content/renderer/peppe... > >> content/renderer/pepper/video_decoder_shim.cc:417: if ((ystride != >> ywidth) || >> (clamp_width_ != ywidth)) { >> I'm not sure I understand the need for this outer if. The if on the line >> below >> makes sure the code does nothing if clamp_width_ == ywidth. >> > > Remove? >> > > I'll refactor this code a bit. > > > > https://codereview.chromium.org/1111653004/diff/120001/content/renderer/peppe... > >> content/renderer/pepper/video_decoder_shim.cc:476: // >> If the format is not supported by the fast path, we should keep the old >> path - >> convert in software directly into the BGRA texture. >> > > I discussed this with Bill before I started and his opinion was it was OK > for > this path to just support planar 420, 422, and 444 - and that a fallback > was not > necessary. Do you want me to add the fallback anyway? If we know the software decoder will never produce anything else, then we should NOTREACHED(). If it can produce something else, then we should have the fallback. I don't think making a red texture is useful. https://codereview.chromium.org/1111653004/diff/120001/content/renderer/peppe... > >> content/renderer/pepper/video_decoder_shim.h:42: class YUVConverter; >> nit: make it an inner class of VideoDecoderShim >> > > Bill asked that I move it out of VideoDecoderShim class in my first CL. > Should > I put it back? content is a big namespace with lots of stuff in it. YUVConverter sounds generic enough for a private class that I don't think it should be in the top level namespace (for fear of ODR violations). > > > > FYI, the reason 2x2 doesn't work is likely because of the default >> GL_UNPACK_ALIGNMENT which is 4. >> > > You are probably right. I'll set the unpack alignment to be 1 for this > section, > since its possible some video formats could be odd widths. > > > https://codereview.chromium.org/1111653004/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
codebythepound@gmail.com changed required reviewers: + piman@chromium.org
Fixed nits. Added support for all the same YUV formats that the software decode path supported.
LGTM with a couple of leftover nits. https://codereview.chromium.org/1111653004/diff/120001/content/renderer/peppe... File content/renderer/pepper/video_decoder_shim.cc (right): https://codereview.chromium.org/1111653004/diff/120001/content/renderer/peppe... content/renderer/pepper/video_decoder_shim.cc:853: // initialize yuv converter On 2015/05/12 03:00:24, piman (Very slow to review) wrote: > nit: remove comment ping https://codereview.chromium.org/1111653004/diff/120001/content/renderer/peppe... content/renderer/pepper/video_decoder_shim.cc:1035: // run the yuv conversion renderer On 2015/05/12 03:00:23, piman (Very slow to review) wrote: > nit: remove comment ping
Fixed remaining issues.
The CQ bit was checked by codebythepound@gmail.com to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org Link to the patchset: https://codereview.chromium.org/1111653004/#ps170003 (title: "Added missing period.")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1111653004/170003
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: All required reviewers (with asterisk prefixes) have not yet approved this CL. No LGTM from a valid reviewer yet. Only full committers are accepted. Even if an LGTM may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
bbudge@chromium.org changed reviewers: + codebythepound@gmail.com - CodeByThePound@gmail.com
https://codereview.chromium.org/1111653004/diff/170003/content/renderer/peppe... File content/renderer/pepper/video_decoder_shim.cc (right): https://codereview.chromium.org/1111653004/diff/170003/content/renderer/peppe... content/renderer/pepper/video_decoder_shim.cc:60: GLuint vtx_buffer_; nit: vertex_buffer_ https://codereview.chromium.org/1111653004/diff/170003/content/renderer/peppe... content/renderer/pepper/video_decoder_shim.cc:90: const scoped_refptr<cc_blink::ContextProviderWebContext>& ctx_p) nit: could you name this context_provider https://codereview.chromium.org/1111653004/diff/170003/content/renderer/peppe... content/renderer/pepper/video_decoder_shim.cc:227: gl_->UseProgram(0); Is there a reason you set these inside the function? It seems like you could set program once, at the call site for these functions below, then reset to 0 once. https://codereview.chromium.org/1111653004/diff/170003/content/renderer/peppe... content/renderer/pepper/video_decoder_shim.cc:315: DCHECK(gl_); DCHECK in the constructor. https://codereview.chromium.org/1111653004/diff/170003/content/renderer/peppe... content/renderer/pepper/video_decoder_shim.cc:382: // YUV nit strange formatting https://codereview.chromium.org/1111653004/diff/170003/content/renderer/peppe... content/renderer/pepper/video_decoder_shim.cc:410: case media::VideoFrame::YV12: /* 420 */ nit: C++ style comments. https://codereview.chromium.org/1111653004/diff/170003/content/renderer/peppe... content/renderer/pepper/video_decoder_shim.cc:583: I think a big comment explaining about restoring state and how we interact with the shared GL context would be helpful for future maintainers. https://codereview.chromium.org/1111653004/diff/170003/content/renderer/peppe... content/renderer/pepper/video_decoder_shim.cc:870: yuv_converter_.reset(new YUVConverter(context_provider_)); nit: this could be initialized sooner, in the initializer list above. (decoder_impl_ can't) https://codereview.chromium.org/1111653004/diff/170003/content/renderer/peppe... content/renderer/pepper/video_decoder_shim.cc:910: return false; Does this mean software fallback won't work in some cases where it did before? https://codereview.chromium.org/1111653004/diff/170003/content/renderer/peppe... content/renderer/pepper/video_decoder_shim.cc:1041: void VideoDecoderShim::OnOutputComplete(scoped_ptr<PendingFrame> pframe) { nit: Chrome style is usually 'frame' https://codereview.chromium.org/1111653004/diff/170003/content/renderer/peppe... File content/renderer/pepper/video_decoder_shim.h (right): https://codereview.chromium.org/1111653004/diff/170003/content/renderer/peppe... content/renderer/pepper/video_decoder_shim.h:113: // Queue of decoded frames that await rgb->yuv conversion nit: period
https://codereview.chromium.org/1111653004/diff/170003/content/renderer/peppe... File content/renderer/pepper/video_decoder_shim.cc (right): https://codereview.chromium.org/1111653004/diff/170003/content/renderer/peppe... content/renderer/pepper/video_decoder_shim.cc:60: GLuint vtx_buffer_; On 2015/05/14 16:45:48, bbudge (OOO week of 5-11) wrote: > nit: vertex_buffer_ Done. https://codereview.chromium.org/1111653004/diff/170003/content/renderer/peppe... content/renderer/pepper/video_decoder_shim.cc:90: const scoped_refptr<cc_blink::ContextProviderWebContext>& ctx_p) On 2015/05/14 16:45:48, bbudge (OOO week of 5-11) wrote: > nit: could you name this context_provider Done. https://codereview.chromium.org/1111653004/diff/170003/content/renderer/peppe... content/renderer/pepper/video_decoder_shim.cc:227: gl_->UseProgram(0); Removed SetUniform functions and inlined the code below. https://codereview.chromium.org/1111653004/diff/170003/content/renderer/peppe... content/renderer/pepper/video_decoder_shim.cc:315: DCHECK(gl_); On 2015/05/14 16:45:48, bbudge (OOO week of 5-11) wrote: > DCHECK in the constructor. Done. https://codereview.chromium.org/1111653004/diff/170003/content/renderer/peppe... content/renderer/pepper/video_decoder_shim.cc:382: // YUV Must have been done by git cl format. Fixed. https://codereview.chromium.org/1111653004/diff/170003/content/renderer/peppe... content/renderer/pepper/video_decoder_shim.cc:410: case media::VideoFrame::YV12: /* 420 */ On 2015/05/14 16:45:48, bbudge (OOO week of 5-11) wrote: > nit: C++ style comments. Done. https://codereview.chromium.org/1111653004/diff/170003/content/renderer/peppe... content/renderer/pepper/video_decoder_shim.cc:583: Added. https://codereview.chromium.org/1111653004/diff/170003/content/renderer/peppe... content/renderer/pepper/video_decoder_shim.cc:870: yuv_converter_.reset(new YUVConverter(context_provider_)); On 2015/05/14 16:45:48, bbudge (OOO week of 5-11) wrote: > nit: this could be initialized sooner, in the initializer list above. > (decoder_impl_ can't) Done. https://codereview.chromium.org/1111653004/diff/170003/content/renderer/peppe... content/renderer/pepper/video_decoder_shim.cc:910: return false; The YUVConverter initialization could fail if 1) the shader failed to compile or 2) there were less than 4 texture units available. OpenGL ES 2.0 spec says a minimum of 8 texture units should be available in the fragment stage, and the shader should be valid everywhere. I could DCHECK these things instead and make Initialize return void. https://codereview.chromium.org/1111653004/diff/170003/content/renderer/peppe... content/renderer/pepper/video_decoder_shim.cc:1041: void VideoDecoderShim::OnOutputComplete(scoped_ptr<PendingFrame> pframe) { On 2015/05/14 16:45:48, bbudge (OOO week of 5-11) wrote: > nit: Chrome style is usually 'frame' Done. https://codereview.chromium.org/1111653004/diff/170003/content/renderer/peppe... File content/renderer/pepper/video_decoder_shim.h (right): https://codereview.chromium.org/1111653004/diff/170003/content/renderer/peppe... content/renderer/pepper/video_decoder_shim.h:113: // Queue of decoded frames that await rgb->yuv conversion On 2015/05/14 16:45:48, bbudge (OOO week of 5-11) wrote: > nit: period Done.
LGTM Thank you piman for picking this up for me. +elijah FYI Software fallback potentially changing to be faster but perhaps less coverage of platforms. https://codereview.chromium.org/1111653004/diff/170003/content/renderer/peppe... File content/renderer/pepper/video_decoder_shim.cc (right): https://codereview.chromium.org/1111653004/diff/170003/content/renderer/peppe... content/renderer/pepper/video_decoder_shim.cc:910: return false; On 2015/05/14 17:42:08, CodeByThePound wrote: > The YUVConverter initialization could fail if 1) the shader failed to compile or > 2) there were less than 4 texture units available. OpenGL ES 2.0 spec says a > minimum of 8 texture units should be available in the fragment stage, and the > shader should be valid everywhere. I could DCHECK these things instead and make > Initialize return void. Let's leave it as is for now. We could add fallback code that used the old skia routine.
Thanks Bill for reviewing while OOO!
The CQ bit was checked by codebythepound@gmail.com to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org Link to the patchset: https://codereview.chromium.org/1111653004/#ps190001 (title: "Further refinements.")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1111653004/190001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by codebythepound@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1111653004/190001
Message was sent while issue was closed.
Committed patchset #11 (id:190001)
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/f5e606219178026623ee38907d14bff14cb83c7b Cr-Commit-Position: refs/heads/master@{#329942}
Message was sent while issue was closed.
Is this better than the gpu based conversion path we have in the compositor today? If so it'd be awesome to integrate it for non-PPAPI video playback too.
Message was sent while issue was closed.
On Thu, May 14, 2015 at 4:15 PM, <dalecurtis@chromium.org> wrote: > Is this better than the gpu based conversion path we have in the compositor > today? If so it'd be awesome to integrate it for non-PPAPI video playback > too. > It's essentially the same. > https://codereview.chromium.org/1111653004/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
