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

Fri Apr 30 20:37:45 PDT 2010

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

--- Comment #54 from Jer Noble <jer.noble at apple.com>  2010-04-30 20:37:45 PST ---
(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()"?

> 
> WebCore/platform/graphics/win/QTMovieVisualContext.cpp:54
>  +  
> We mostly use get for functions that have out parameters. This should be
> currentHostTime() and hostClockFrequency(). Can these be const?

Better, they can be static.  Also, I'll just combine those two calls into one
which does the division and returns a CFTimeInterval.

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

PassRefPtr instead.

> WebCore/platform/graphics/win/QTMovieVisualContext.cpp:55
>  +      void setMovie(RefPtr<QTMovie>);
> This should take a PassRefPtr to reduce ref churn.

Yup.

> WebCore/platform/graphics/win/QTMovieVisualContext.cpp:73
>  +      HMODULE qtmlDLL = ::LoadLibraryW(L"QTMLClient.dll");
> Should this be a static variable? Is it OK to load the library everytime a
> QTMovieVisualContextPriv object is created?

It absolutely can be.  LoadLibrary won't do any extra work if the DLL is
already loaded, but it should be static anyway.

> WebCore/platform/graphics/win/QTMovieVisualContext.cpp:74
>  +      if (qtmlDLL) {
> An early return here would reduce indentation.
> 
> WebCore/platform/graphics/win/QTMovieVisualContext.cpp:105
>  +              CVPixelBufferRelease(newImage);
> Could QTPixelBuffer have an "adopt" member function that would take a
> reference? If so the CVPixelBufferRelease call could be omitted.

Yep.

> WebCore/platform/graphics/win/QTMovieVisualContext.cpp:134
>  +  
> Yup, looks like this could take a PassRefPtr.

Ok.

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

Why not a return PassRefPtr?

> 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.

> 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.

> WebCore/platform/graphics/win/QTMovieVisualContext.h:66
>  +      double getHostClockFrequency();
> Same comments here as in QTMovieVisualContextPrivate.

Yep.

> WebCore/platform/graphics/win/QTMovieVisualContext.h:70
>  +  
> Same comments here as in QTMovieVisualContextPrivate.

Yep.

> WebCore/platform/graphics/win/QTPixelBuffer.cpp:35
>  +  QTPixelBuffer::QTPixelBuffer() : m_pixelBuffer(0) {}
> member initializers should go on a separate row.

Ok.

> WebCore/platform/graphics/win/QTPixelBuffer.cpp:37
>  +  QTPixelBuffer::QTPixelBuffer(const QTPixelBuffer& p) :
> m_pixelBuffer(p.m_pixelBuffer) 
> Ditto.

Ok.

> WebCore/platform/graphics/win/QTPixelBuffer.cpp:42
>  +  QTPixelBuffer::QTPixelBuffer(CVPixelBufferRef ref) : m_pixelBuffer(ref)
> Ditto.

Ok.

> WebCore/platform/graphics/win/QTPixelBuffer.cpp:68
>  +  }
> Should be called "pixelBuffer" or something other than get.

Similarly, perhaps "pixelBufferRef()"?

> WebCore/platform/graphics/win/QTPixelBuffer.cpp:74
>  +  }
> Does CVPixelBufferRelease work if m_pixelBuffer is 0?

Yep.

> WebCore/platform/graphics/win/QTPixelBuffer.h:62
>  +  
> Can width and height be const?

Same as above, yes.

> WebCore/platform/graphics/win/QTPixelBuffer.h:69
>  +  
> Can these be const? (Or at least some of them).

All of the "consty" ones can, yes.

> WebCore/platform/graphics/win/QTPixelBuffer.h:76
>  +  
> Const?

Sure.

> 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.

> WebCore/platform/graphics/win/QTPixelBuffer.cpp:166
>  +      return propogated;
> Can just merge these into a single line. 

Sure thing.  (There used to be a CFShow() between those two lines, for
debugging purposes.)

> 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.  

> 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.

> 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.

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