[Webkit-unassigned] [Bug 34005] Safari pegs CPU and drops tons of frames using HTML5 Vimeo player

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=34005


Anders Carlsson <andersca at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |andersca at apple.com




--- Comment #55 from Anders Carlsson <andersca at apple.com>  2010-05-03 10:20:16 PST ---
(In reply to comment #54)
> (In reply to comment #51)
> > WebCore/platform/graphics/win/QTMovieVisualContext.cpp:50
> >  +      QTVisualContextRef get();
> > I think this should be renamed to visualContext() or something other than
> > get().
> 
> Originally, I didn't like the looks of foo->visualContext()->visualContext(). 
> (If the class "Foo" had an accessor which returned a QTVisualContext*.)  I
> considered "platformVisualContext()" or "rawVisualContext()" but didn't like
> any of those either.  "get()" was a cop-out.
> 
> How about "visualContextRef()"?

I like that!

> > WebCore/platform/graphics/win/QTMovieVisualContext.cpp:56
> >  +      RefPtr<QTMovie> movie() const;
> > No need for this to return a RefPtr.
> 
> PassRefPtr instead.

You don't want it to return a PassRefPtr, because then if someone does

context->movie()->doSomething()

that's going to ref and then deref the QTMovie object for no good reason.

> > WebCore/platform/graphics/win/QTMovieVisualContext.cpp:138
> >  +  }
> > And this could return a QTMovie * instead of a RefPtr.
> 
> Why not a return PassRefPtr?

See my comment above.

> 
> > WebCore/platform/graphics/win/QTMovieVisualContext.cpp:47
> >  +      QTPixelBuffer imageForTime(const QTCVTimeStamp*);
> > Can these be const?
> 
> Originally, I had thought that if they were, this->m_visualContext would be a
> "const CVPixelBufferRef", and I'd have to const_cast away the constness before
> passing it on to QTVisualContextIsImageAvailableForTime.  Apparently, (I just
> tried) this compiles just fine without the const_cast.   So, yes. :)
> 
> However, calling QTVisualContextCopyImageForTime may change the internal state
> of the QTVisualContext, so it's not necessarily a "const" operation.
>

Makes sense!

> > WebCore/platform/graphics/win/QTMovieVisualContext.cpp:123
> >  +  {
> > You could return early here if (movie == m_movie). (Especially when movie is a
> > PassRefPtr).
> 
> Yep.
> 
> > WebCore/platform/graphics/win/QTMovieVisualContext.h:63
> >  +      QTVisualContextRef get();
> > Again, a better name for this would be visualContext().
> 
> Yep.
> 

Or visualContextRef() :)

> > WebCore/platform/graphics/win/QTPixelBuffer.cpp:68
> >  +  }
> > Should be called "pixelBuffer" or something other than get.
> 
> Similarly, perhaps "pixelBufferRef()"?
> 

Sounds good.

> > WebCore/platform/graphics/win/QTPixelBuffer.h:77
> >  +      void getExtendedPixels(size_t* left, size_t* right, size_t* top,
> > size_t* bottom);
> > Maybe references would be better than pointers here?
> 
> The references will just be turned into pointers when we call
> CVPixelBufferGetExtendedPixels.  And if the caller only wanted to get the
> "left" extended pixel, they would still have to define and pass in all the
> other parameters anyway.

OK!

> 
> > WebCore/platform/graphics/win/QTPixelBuffer.h:51
> >  +      QTPixelBuffer& operator=(const QTPixelBuffer&);
> > Come to think of it, I don't think ref counted objects should be copyable. Are
> > the copy constructor and copy-assignment operators really needed?
> 
> Nope.  These could be moved into private: and unimplemented.  

The way we do this is by inheriting from the "Noncopyable" object.

> 
> > WebCore/platform/graphics/win/QTPixelBuffer.h:82
> >  +      static void retainCallback(void* refcon);
> > What are these used for?
> 
> Funny story.  You can't pass a CVPixelBuffer ref created in QTMovieWin.dll to a
> CFRetain or CFRelease defined in CoreFoundation.dll.  So whenever a retain or a
> release needs to be called on a CVPixelBufferRef, it needs to occur within
> QTMovieWin.dll.  
> 
> I defined three different types of retain/release callbacks in the
> QTPixelBuffer class: a straight CF-style retain/release callback that takes a
> single parameter, a CAImageQueue style release callback that takes a few other
> extraneous parameters, and a CGDataProvider style release callback.  
> 
> The CF-style retain/release callbacks are so you can put a CVPixelBufferRef in
> a CFArray (with the correct retain/release callbacks passed in during CFArray
> creation).  The CAImageQueue callbacks are for use with CAImageQueues (which
> don't take CVPixelBuffers directly, but want a callback they can use to release
> the underlying pixel data).  And the CGDataProvider callbacks are so the
> QTPixelBuffers can be used to create CGImages, backed by CGDataProviders,
> referencing the original pixel data in the CVPixelBuffer.
> 

Ah, I see!

> > Patch looks good overall, but I'd like to see my comments addressed in a new
> > patch.
> 
> Sure thing.  I'll post a replacement patch.

Great!

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.