Repaint video controls when buffering during pause.

Pipes support for a new bufferringProgressed() method on MediaControls elements.

HTMLMediaElement runs a timer which notifies the controls via the new method whenever new data has been buffered.

I've added a new layout test to verify this behavior by checking for repaints between 'progress' events.

BUG=97720
TEST=Loaded video, paused, watched buffer progress. New layout test, existing layout tests.

[DaleCurtis, 2011-11-09 01:47:17]

PTAL. I may have over thought the problem trying to keep the NetworkEvent rate
as close to pre-change as possible by only forwarding/repainting when paused.

[scherkus (not reviewing), 2011-11-09 03:45:27]

+vrk who knows this better

main question for vrk is: is it correct to keep calling the network event
callback or do we need something fancier like a "amount buffered has changed"
callback?

http://codereview.chromium.org/8496044/diff/1/webkit/glue/media/buffered_data...
File webkit/glue/media/buffered_data_source.cc (right):

http://codereview.chromium.org/8496044/diff/1/webkit/glue/media/buffered_data...
webkit/glue/media/buffered_data_source.cc:647: } else if (media_is_paused_ &&
host())
nit: if one if branch has {} then all branches get {}

http://codereview.chromium.org/8496044/diff/1/webkit/glue/webmediaplayer_impl.cc
File webkit/glue/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/8496044/diff/1/webkit/glue/webmediaplayer_impl...
webkit/glue/webmediaplayer_impl.cc:808: } else
nit: use return and exit early instead of else

[scherkus (not reviewing), 2011-11-09 03:47:32]

one more comment

http://codereview.chromium.org/8496044/diff/1/webkit/glue/webmediaplayer_impl.cc
File webkit/glue/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/8496044/diff/1/webkit/glue/webmediaplayer_impl...
webkit/glue/webmediaplayer_impl.cc:807: Repaint();
also I wonder whether a repaint() here is the right course of action

there's a slim possibility that it'll chew up resources while
paused-yet-buffering

is there something we can call on GetClient() to signal that the amount buffered
has changed? perhaps that way we can issue a repaint for just the controls as
opposed to the entire video surface

note that we can add/remove methods to WebMediaPlayerClient interface if need be

[DaleCurtis, 2011-11-09 18:30:01]

http://codereview.chromium.org/8496044/diff/1/webkit/glue/media/buffered_data...
File webkit/glue/media/buffered_data_source.cc (right):

http://codereview.chromium.org/8496044/diff/1/webkit/glue/media/buffered_data...
webkit/glue/media/buffered_data_source.cc:647: } else if (media_is_paused_ &&
host())
On 2011/11/09 03:45:28, scherkus wrote:
> nit: if one if branch has {} then all branches get {}

Done.

http://codereview.chromium.org/8496044/diff/1/webkit/glue/webmediaplayer_impl.cc
File webkit/glue/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/8496044/diff/1/webkit/glue/webmediaplayer_impl...
webkit/glue/webmediaplayer_impl.cc:807: Repaint();
Would a comparison to host->GetBufferedBytes() and buffered_bytes_ prior to the
host->SetBufferedBytes(...)  call in BufferedDataSource::NetworkEventCallback()
be sufficient?

When paused these should be monotonically increasing right?

On 2011/11/09 03:47:33, scherkus wrote:
> also I wonder whether a repaint() here is the right course of action
> 
> there's a slim possibility that it'll chew up resources while
> paused-yet-buffering
> 
> is there something we can call on GetClient() to signal that the amount
buffered
> has changed? perhaps that way we can issue a repaint for just the controls as
> opposed to the entire video surface
> 
> note that we can add/remove methods to WebMediaPlayerClient interface if need
be

http://codereview.chromium.org/8496044/diff/1/webkit/glue/webmediaplayer_impl...
webkit/glue/webmediaplayer_impl.cc:808: } else
On 2011/11/09 03:45:28, scherkus wrote:
> nit: use return and exit early instead of else

Done.

[vrk (LEFT CHROMIUM), 2011-11-09 23:42:13]

On 2011/11/09 03:45:27, scherkus wrote:
> +vrk who knows this better
> 
> main question for vrk is: is it correct to keep calling the network event
> callback or do we need something fancier like a "amount buffered has changed"
> callback?

In short: no, we shouldn't keep calling SetNetworkActivity. SetNetworkActivity
is meant to signal when the download defers and continues, so modifying
BufferedDataSource in this way is not the way to go.

I was going to suggest modifying PipelineImpl to notify WebMediaPlayerImpl to
repaint every time SetBufferedBytes is called, but acolwell had a simple and
interesting alternate idea: add a timer to WebMediaPlayerImpl that calls
Repaint() every X ms when it is paused and network_state_ == loading. We don't
care about updating the buffered bar super accurately or and we also want to
avoid repainting too often, so I think this is a good approach.

dale/scherkus, thoughts?

[DaleCurtis, 2011-11-09 23:54:19]

The timer solution seems opaque, I'd prefer the Repaint() had a corresponding
fixed event point like either BufferedBytes or NetworkActivity.

I'm up for implementing some new piping, although I'd argue that
SetNetworkActivity is named such that it implies a callback occurs when there is
new network activity. Can you provide the long answer on why a network event
shouldn't be issued when new data is received?

Thanks!

On 2011/11/09 23:42:13, Victoria Kirst wrote:
> On 2011/11/09 03:45:27, scherkus wrote:
> > +vrk who knows this better
> > 
> > main question for vrk is: is it correct to keep calling the network event
> > callback or do we need something fancier like a "amount buffered has
changed"
> > callback?
> 
> In short: no, we shouldn't keep calling SetNetworkActivity. SetNetworkActivity
> is meant to signal when the download defers and continues, so modifying
> BufferedDataSource in this way is not the way to go.
> 
> I was going to suggest modifying PipelineImpl to notify WebMediaPlayerImpl to
> repaint every time SetBufferedBytes is called, but acolwell had a simple and
> interesting alternate idea: add a timer to WebMediaPlayerImpl that calls
> Repaint() every X ms when it is paused and network_state_ == loading. We don't
> care about updating the buffered bar super accurately or and we also want to
> avoid repainting too often, so I think this is a good approach.
> 
> dale/scherkus, thoughts?

[vrk (LEFT CHROMIUM), 2011-11-10 00:33:28]

On 2011/11/09 23:54:19, DaleCurtis wrote:
> The timer solution seems opaque, I'd prefer the Repaint() had a corresponding
> fixed event point like either BufferedBytes or NetworkActivity.

Why do you prefer that? What benefit is there to having a fixed event point? 

(For what it's worth, in the existing code path there is no explicit call to
issue a repaint of the buffering bar. The buffering bar gets repainted as a
consequence of issuing a call to paint a new video frame.)

> I'm up for implementing some new piping, although I'd argue that
> SetNetworkActivity is named such that it implies a callback occurs when there
is
> new network activity. Can you provide the long answer on why a network event
> shouldn't be issued when new data is received?

New data can be received very rapidly and in small increments, so signalling a
repaint every time this is updated can result in a lot of unnecessary repaints
and thread traffic. You could throttle these repaint requests, but using a timer
inherently throttles for you and is simple to implement -- which is not only
easier for you, but it also avoids further complication of our already-complex
pipeline code! :) 

Because there is no need to have a precise buffering bar, I can see no advantage
to calling Repaint() on every network event.

[DaleCurtis, 2011-11-10 02:02:55]

1. I prefer it because doing otherwise is diverging from the event driven system
which seems prevalent throughout the code base. Are you using a similar
mechanism anywhere else in the code? Adding a new mechanism is arguably more
complex than following existing practices.

2. The threaded approach isn't cut and dry either, care will need to be taken to
make sure I don't introduce any race conditions during shutdown.

3. Repaints are already limited to a maximum of 50 outstanding requests in
WebMediaPlayerProxy. I'm not sure, but Vsync should keep this from ever being a
crazy number per sec.

In any case, I defer to your judgement. If my arguments have not swayed you I'll
happily implement the threaded approach. I'm always happy to take the easier
route :)

On 2011/11/10 00:33:28, Victoria Kirst wrote:
> On 2011/11/09 23:54:19, DaleCurtis wrote:
> > The timer solution seems opaque, I'd prefer the Repaint() had a
corresponding
> > fixed event point like either BufferedBytes or NetworkActivity.
> 
> Why do you prefer that? What benefit is there to having a fixed event point? 
> 
> (For what it's worth, in the existing code path there is no explicit call to
> issue a repaint of the buffering bar. The buffering bar gets repainted as a
> consequence of issuing a call to paint a new video frame.)
> 
> > I'm up for implementing some new piping, although I'd argue that
> > SetNetworkActivity is named such that it implies a callback occurs when
there
> is
> > new network activity. Can you provide the long answer on why a network event
> > shouldn't be issued when new data is received?
> 
> New data can be received very rapidly and in small increments, so signalling a
> repaint every time this is updated can result in a lot of unnecessary repaints
> and thread traffic. You could throttle these repaint requests, but using a
timer
> inherently throttles for you and is simple to implement -- which is not only
> easier for you, but it also avoids further complication of our already-complex
> pipeline code! :) 
> 
> Because there is no need to have a precise buffering bar, I can see no
advantage
> to calling Repaint() on every network event.

[vrk (LEFT CHROMIUM), 2011-11-10 04:19:57]

Your saying "threaded" approach makes me think there may be a misunderstanding
about the suggestion :) My thought was you would add a RepeatingTimer to
WebMediaPlayerImpl, and OnNetworkEvent you'd start the timer when paused and
loading, and stop the timer when unpaused, loading stopped, or destroyed... is
this what you were thinking, too?

> 3. Repaints are already limited to a maximum of 50 outstanding requests in
> WebMediaPlayerProxy. I'm not sure, but Vsync should keep this from ever being
a
> crazy number per sec.

Actually, WebMediaPlayerProxy is proxying things from other threads *to*
WebMediaPlayerImpl! So it's limiting the number of times it calls
WebMediaPlayerImpl::Repaint(), not the other direction. That means when calling
Repaint() in WebMediaPlayerImpl directly, we're on our own for limiting those
calls.

We can chat more in person about options! Would be good to hear what scherkus
thinks, too.

[vrk (LEFT CHROMIUM), 2011-11-10 04:27:02]

> Actually, WebMediaPlayerProxy is proxying things from other threads *to*
> WebMediaPlayerImpl! 

Err crap, I should qualify that: WebMediaPlayerProxy is proxying things from
other threads to WebMediaPlayerImpl in the Repaint scenario. It also proxies
outgoing calls from WebMediaPlayerImpl to the filters. 

At any rate, it's a proxy between WebMediaPlayerImpl and pipeline/filters, not
WebMediaPlayerImpl and its WebKit-residing client.

[DaleCurtis, 2011-11-10 06:45:37]

Ah, yes, definitely a misunderstanding on my part. My apologies! When you said
timer I read thread. Your solution sounds perfect! If scherkus agrees, I'll go
ahead and write that up tomorrow.

Whoops, thanks for the correction! I'll have to dig through that code more
carefully. Thanks for your insight and patience :)

On 2011/11/10 04:19:57, Victoria Kirst wrote:
> Your saying "threaded" approach makes me think there may be a misunderstanding
> about the suggestion :) My thought was you would add a RepeatingTimer to
> WebMediaPlayerImpl, and OnNetworkEvent you'd start the timer when paused and
> loading, and stop the timer when unpaused, loading stopped, or destroyed... is
> this what you were thinking, too?
> 
> > 3. Repaints are already limited to a maximum of 50 outstanding requests in
> > WebMediaPlayerProxy. I'm not sure, but Vsync should keep this from ever
being
> a
> > crazy number per sec.
> 
> Actually, WebMediaPlayerProxy is proxying things from other threads *to*
> WebMediaPlayerImpl! So it's limiting the number of times it calls
> WebMediaPlayerImpl::Repaint(), not the other direction. That means when
calling
> Repaint() in WebMediaPlayerImpl directly, we're on our own for limiting those
> calls.
> 
> We can chat more in person about options! Would be good to hear what scherkus
> thinks, too.

[scherkus (not reviewing), 2011-11-10 23:35:33]

A timer seems screwy but work.

Did we rule out the "have webkit notice a difference in buffered bytes and tell
the shadow DOM controls to update themselves" approach?

[vrk (LEFT CHROMIUM), 2011-11-10 23:58:10]

On 2011/11/10 23:35:33, scherkus wrote:
> A timer seems screwy but work.
> 
> Did we rule out the "have webkit notice a difference in buffered bytes and
tell
> the shadow DOM controls to update themselves" approach?

Didn't rule out! I tried looking into that code yesterday but didn't explore far
enough to be able to formulate a plan... Here's what I did see:

 - When the progress timer fires (HTMLMediaElement::progressEventTimerFired), it
detects progress by looking for diffs in bytes_loaded()
 - If there is a diff, then it calls renderer()->updateFromElement(), which in
video's case calls RenderVideo::updatePlayer(). This appears to make changes to
the MediaPlayer's state but does not issue a repaint.

It would seem like this could be the place to issue a repaint for controls,
though I'm not sure exactly how to do that. However, the comment (!) in
RenderObject.h for updateFromElement() is:
  // used for element state updates that cannot be fixed with a
  // repaint and do not need a relayout
So it seems like a repaint would be violating the contract of
updateFromElement()? But who knows; I don't know what most render objects use
updateFromElement for.

If the timer seems too weird, I think it's worth doing more exploration in
WebKit... WebKit rendering is beastly though!

[DaleCurtis, 2011-11-11 00:33:59]

The issue is not fixed in sjl's patch.

On 2011/11/10 23:58:10, Victoria Kirst wrote:
> On 2011/11/10 23:35:33, scherkus wrote:
> > A timer seems screwy but work.
> > 
> > Did we rule out the "have webkit notice a difference in buffered bytes and
> tell
> > the shadow DOM controls to update themselves" approach?
> 
> Didn't rule out! I tried looking into that code yesterday but didn't explore
far
> enough to be able to formulate a plan... Here's what I did see:
> 
>  - When the progress timer fires (HTMLMediaElement::progressEventTimerFired),
it
> detects progress by looking for diffs in bytes_loaded()
>  - If there is a diff, then it calls renderer()->updateFromElement(), which in
> video's case calls RenderVideo::updatePlayer(). This appears to make changes
to
> the MediaPlayer's state but does not issue a repaint.
> 
> It would seem like this could be the place to issue a repaint for controls,
> though I'm not sure exactly how to do that. However, the comment (!) in
> RenderObject.h for updateFromElement() is:
>   // used for element state updates that cannot be fixed with a
>   // repaint and do not need a relayout
> So it seems like a repaint would be violating the contract of
> updateFromElement()? But who knows; I don't know what most render objects use
> updateFromElement for.
> 
> If the timer seems too weird, I think it's worth doing more exploration in
> WebKit... WebKit rendering is beastly though!

[DaleCurtis, 2011-11-11 19:23:49]

So using Victoria's pointers, I followed this call all the way back to
WebMediaPlayerImpl::setVisible(bool). However when I issue a repaint during the
setVisible call, it doesn't actually work... Any ideas?

On 2011/11/11 00:33:59, DaleCurtis wrote:
> The issue is not fixed in sjl's patch.
> 
> On 2011/11/10 23:58:10, Victoria Kirst wrote:
> > On 2011/11/10 23:35:33, scherkus wrote:
> > > A timer seems screwy but work.
> > > 
> > > Did we rule out the "have webkit notice a difference in buffered bytes and
> > tell
> > > the shadow DOM controls to update themselves" approach?
> > 
> > Didn't rule out! I tried looking into that code yesterday but didn't explore
> far
> > enough to be able to formulate a plan... Here's what I did see:
> > 
> >  - When the progress timer fires
(HTMLMediaElement::progressEventTimerFired),
> it
> > detects progress by looking for diffs in bytes_loaded()
> >  - If there is a diff, then it calls renderer()->updateFromElement(), which
in
> > video's case calls RenderVideo::updatePlayer(). This appears to make changes
> to
> > the MediaPlayer's state but does not issue a repaint.
> > 
> > It would seem like this could be the place to issue a repaint for controls,
> > though I'm not sure exactly how to do that. However, the comment (!) in
> > RenderObject.h for updateFromElement() is:
> >   // used for element state updates that cannot be fixed with a
> >   // repaint and do not need a relayout
> > So it seems like a repaint would be violating the contract of
> > updateFromElement()? But who knows; I don't know what most render objects
use
> > updateFromElement for.
> > 
> > If the timer seems too weird, I think it's worth doing more exploration in
> > WebKit... WebKit rendering is beastly though!

[DaleCurtis, 2011-11-16 00:40:32]

I was doing something wrong apparently. I retried my new fix today and it worked
properly. PTAL.

[scherkus (not reviewing), 2011-11-16 02:09:23]

http://codereview.chromium.org/8496044/diff/14001/webkit/glue/webmediaplayer_...
File webkit/glue/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/8496044/diff/14001/webkit/glue/webmediaplayer_...
webkit/glue/webmediaplayer_impl.cc:399: // RenderVideo->updateFromElement()
which ends in a call to here.
I think the part I'm more interested in is when/why does it issue this call?

http://codereview.chromium.org/8496044/diff/14001/webkit/glue/webmediaplayer_...
webkit/glue/webmediaplayer_impl.cc:403: // TODO(hclam): add appropriate method
call when pipeline has it implemented.
do you know what this TODO might be referring to?

perhaps it's time to remove it!

[DaleCurtis, 2011-11-16 02:31:48]

http://codereview.chromium.org/8496044/diff/14001/webkit/glue/webmediaplayer_...
File webkit/glue/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/8496044/diff/14001/webkit/glue/webmediaplayer_...
webkit/glue/webmediaplayer_impl.cc:399: // RenderVideo->updateFromElement()
which ends in a call to here.
On 2011/11/16 02:09:25, scherkus wrote:
> I think the part I'm more interested in is when/why does it issue this call?

Done.

http://codereview.chromium.org/8496044/diff/14001/webkit/glue/webmediaplayer_...
webkit/glue/webmediaplayer_impl.cc:403: // TODO(hclam): add appropriate method
call when pipeline has it implemented.
I sent him an email. I'll report back.

On 2011/11/16 02:09:25, scherkus wrote:
> do you know what this TODO might be referring to?
> 
> perhaps it's time to remove it!

[scherkus (not reviewing), 2011-11-16 02:33:41]

LGTM

[scherkus (not reviewing), 2011-11-16 03:10:05]

hold the phone -- before committing you should:
  a) Update CL description to match latest change
  b) Run layout tests to see if this is going to cause any pixel failures

Also:
  c) Consider how we could get coverage via layout test -- this might
challenging but I might know of a way

[DaleCurtis, 2011-11-18 01:41:01]

I've run into a strange problem, my fixes work via NX but not locally.

The Repaint() call traces to RenderObject::repaintUsingContainer(...):

https://cs.corp.google.com/#chrome/src/third_party/WebKit/Source/WebCore/rend...

Over NX it now passes into the repaintContainer->isRenderView(). However
locally, it passes into v->usesCompositing(), a bit further down the line it
ends here:

https://cs.corp.google.com/#chrome/src/third_party/WebKit/Source/WebCore/rend...

Where nothing happens because, m_graphicsLayer->drawsContent() is false when the
video paused. But! Even after setting this to True, the controls still don't
update. I guessed that the media controls are a separate element than the video
element. So took another route...

But! Even after piping a call to mediaControls()->updateStatusDisplay() from
HTMLMediaElement progressEventTimerFired and issuing a
m_mediaControls->renderer()->updateFromElement() during updateStatusDisplay()
they still don't redraw :(

Any ideas?

On 2011/11/16 03:10:05, scherkus wrote:
> hold the phone -- before committing you should:
>   a) Update CL description to match latest change
>   b) Run layout tests to see if this is going to cause any pixel failures
> 
> Also:
>   c) Consider how we could get coverage via layout test -- this might
> challenging but I might know of a way

[DaleCurtis, 2011-11-18 01:58:35]

After writing this I found a solution which works, instead of calling
m_mediaControls->renderer()->updateFromElement() during updateStatusDisplay() I
instead call m_timeline->setPosition(m_mediaElement->currentTime())

Still unsure if that's the right way, but will update CL shortly :)


On 2011/11/18 01:41:01, DaleCurtis wrote:
> I've run into a strange problem, my fixes work via NX but not locally.
> 
> The Repaint() call traces to RenderObject::repaintUsingContainer(...):
> 
>
https://cs.corp.google.com/#chrome/src/third_party/WebKit/Source/WebCore/rend...
> 
> Over NX it now passes into the repaintContainer->isRenderView(). However
> locally, it passes into v->usesCompositing(), a bit further down the line it
> ends here:
> 
>
https://cs.corp.google.com/#chrome/src/third_party/WebKit/Source/WebCore/rend...
> 
> Where nothing happens because, m_graphicsLayer->drawsContent() is false when
the
> video paused. But! Even after setting this to True, the controls still don't
> update. I guessed that the media controls are a separate element than the
video
> element. So took another route...
> 
> But! Even after piping a call to mediaControls()->updateStatusDisplay() from
> HTMLMediaElement progressEventTimerFired and issuing a
> m_mediaControls->renderer()->updateFromElement() during updateStatusDisplay()
> they still don't redraw :(
> 
> Any ideas?
> 
> On 2011/11/16 03:10:05, scherkus wrote:
> > hold the phone -- before committing you should:
> >   a) Update CL description to match latest change
> >   b) Run layout tests to see if this is going to cause any pixel failures
> > 
> > Also:
> >   c) Consider how we could get coverage via layout test -- this might
> > challenging but I might know of a way

[DaleCurtis, 2011-11-18 02:17:43]

PTAL. CL updated to show what I did. Although these files wouldn't be committed
in our repo. Feel free to clear LGTM.

On 2011/11/18 01:58:35, DaleCurtis wrote:
> After writing this I found a solution which works, instead of calling
> m_mediaControls->renderer()->updateFromElement() during updateStatusDisplay()
I
> instead call m_timeline->setPosition(m_mediaElement->currentTime())
> 
> Still unsure if that's the right way, but will update CL shortly :)
> 
> 
> On 2011/11/18 01:41:01, DaleCurtis wrote:
> > I've run into a strange problem, my fixes work via NX but not locally.
> > 
> > The Repaint() call traces to RenderObject::repaintUsingContainer(...):
> > 
> >
>
https://cs.corp.google.com/#chrome/src/third_party/WebKit/Source/WebCore/rend...
> > 
> > Over NX it now passes into the repaintContainer->isRenderView(). However
> > locally, it passes into v->usesCompositing(), a bit further down the line it
> > ends here:
> > 
> >
>
https://cs.corp.google.com/#chrome/src/third_party/WebKit/Source/WebCore/rend...
> > 
> > Where nothing happens because, m_graphicsLayer->drawsContent() is false when
> the
> > video paused. But! Even after setting this to True, the controls still don't
> > update. I guessed that the media controls are a separate element than the
> video
> > element. So took another route...
> > 
> > But! Even after piping a call to mediaControls()->updateStatusDisplay() from
> > HTMLMediaElement progressEventTimerFired and issuing a
> > m_mediaControls->renderer()->updateFromElement() during
updateStatusDisplay()
> > they still don't redraw :(
> > 
> > Any ideas?
> > 
> > On 2011/11/16 03:10:05, scherkus wrote:
> > > hold the phone -- before committing you should:
> > >   a) Update CL description to match latest change
> > >   b) Run layout tests to see if this is going to cause any pixel failures
> > > 
> > > Also:
> > >   c) Consider how we could get coverage via layout test -- this might
> > > challenging but I might know of a way

[DaleCurtis, 2011-11-22 20:22:09]

Ping.

On 2011/11/18 02:17:43, DaleCurtis wrote:
> PTAL. CL updated to show what I did. Although these files wouldn't be
committed
> in our repo. Feel free to clear LGTM.
> 
> On 2011/11/18 01:58:35, DaleCurtis wrote:
> > After writing this I found a solution which works, instead of calling
> > m_mediaControls->renderer()->updateFromElement() during
updateStatusDisplay()
> I
> > instead call m_timeline->setPosition(m_mediaElement->currentTime())
> > 
> > Still unsure if that's the right way, but will update CL shortly :)
> > 
> > 
> > On 2011/11/18 01:41:01, DaleCurtis wrote:
> > > I've run into a strange problem, my fixes work via NX but not locally.
> > > 
> > > The Repaint() call traces to RenderObject::repaintUsingContainer(...):
> > > 
> > >
> >
>
https://cs.corp.google.com/#chrome/src/third_party/WebKit/Source/WebCore/rend...
> > > 
> > > Over NX it now passes into the repaintContainer->isRenderView(). However
> > > locally, it passes into v->usesCompositing(), a bit further down the line
it
> > > ends here:
> > > 
> > >
> >
>
https://cs.corp.google.com/#chrome/src/third_party/WebKit/Source/WebCore/rend...
> > > 
> > > Where nothing happens because, m_graphicsLayer->drawsContent() is false
when
> > the
> > > video paused. But! Even after setting this to True, the controls still
don't
> > > update. I guessed that the media controls are a separate element than the
> > video
> > > element. So took another route...
> > > 
> > > But! Even after piping a call to mediaControls()->updateStatusDisplay()
from
> > > HTMLMediaElement progressEventTimerFired and issuing a
> > > m_mediaControls->renderer()->updateFromElement() during
> updateStatusDisplay()
> > > they still don't redraw :(
> > > 
> > > Any ideas?
> > > 
> > > On 2011/11/16 03:10:05, scherkus wrote:
> > > > hold the phone -- before committing you should:
> > > >   a) Update CL description to match latest change
> > > >   b) Run layout tests to see if this is going to cause any pixel
failures
> > > > 
> > > > Also:
> > > >   c) Consider how we could get coverage via layout test -- this might
> > > > challenging but I might know of a way

[vrk (LEFT CHROMIUM), 2011-11-22 22:39:03]

Hmm, unfortunately, it looks like "Status Display" is a reference to a
particular control element, just like how there's a time display that shows the
play time. "Status Display," I believe, is an element that would say something
like "loading," "streaming" etc. depending on the status of the media. Chrome's
default controls do not have this, which is why the
MediaControlRootElementChromium overrides the method to do nothing.

But looks like you can call mediaControls()->playbackprogressed() instead?

[vrk (LEFT CHROMIUM), 2011-11-22 22:42:59]

On 2011/11/22 22:39:03, Victoria Kirst wrote:
> But looks like you can call mediaControls()->playbackprogressed() instead?

Ah whoops! Yeah you can't just call *playback* progressed, since you're paused
:) but I think you can do some tweaking around the area, like either
generalizing the existing playbackProgressed() method to deal with both paused
and playing cases, or making a new bufferingProgressed() method, or something!

[DaleCurtis, 2011-11-22 22:46:09]

Sure that would work as well. Is this the right approach in general though?
1. Adding a call out to the controls in HTMLMediaElement. Should I add a pause
check before calling out?
2. Setting the playback progressed instead of piping a more formal repaint
solution? (how?)

On 2011/11/22 22:39:03, Victoria Kirst wrote:
> Hmm, unfortunately, it looks like "Status Display" is a reference to a
> particular control element, just like how there's a time display that shows
the
> play time. "Status Display," I believe, is an element that would say something
> like "loading," "streaming" etc. depending on the status of the media.
Chrome's
> default controls do not have this, which is why the
> MediaControlRootElementChromium overrides the method to do nothing.
> 
> But looks like you can call mediaControls()->playbackprogressed() instead?

[DaleCurtis, 2011-11-22 22:57:48]

Piping a bufferingProgressed() method seems like the right idea. However, it
also sounds like something I need to ask upstream for permission for first. Who
is the right person to talk to about that?

On 2011/11/22 22:46:09, DaleCurtis wrote:
> Sure that would work as well. Is this the right approach in general though?
> 1. Adding a call out to the controls in HTMLMediaElement. Should I add a pause
> check before calling out?
> 2. Setting the playback progressed instead of piping a more formal repaint
> solution? (how?)
> 
> On 2011/11/22 22:39:03, Victoria Kirst wrote:
> > Hmm, unfortunately, it looks like "Status Display" is a reference to a
> > particular control element, just like how there's a time display that shows
> the
> > play time. "Status Display," I believe, is an element that would say
something
> > like "loading," "streaming" etc. depending on the status of the media.
> Chrome's
> > default controls do not have this, which is why the
> > MediaControlRootElementChromium overrides the method to do nothing.
> > 
> > But looks like you can call mediaControls()->playbackprogressed() instead?

[vrk (LEFT CHROMIUM), 2011-11-22 23:23:03]

On 2011/11/22 22:46:09, DaleCurtis wrote:
> Sure that would work as well. Is this the right approach in general though?
It looks right to me, though I'm not an expert in these parts!

On 2011/11/22 22:57:48, DaleCurtis wrote:
> Piping a bufferingProgressed() method seems like the right idea. However, it
> also sounds like something I need to ask upstream for permission for first.
Who
> is the right person to talk to about that?

Since your change is small and the code is easy to understand, I think it makes
more sense to just upload a WebKit patch with your proposed fix. Then you could
run it by Eric Carlson (Apple) and ask for his thoughts on the approach, and if
it looks good to him, he will have the WebKit commit-queue land it.

If you want, you can upload your changes here again and get an unofficial LGTM
from me/scherkus before sending to WebKit!

[scherkus (not reviewing), 2011-11-23 01:41:09]

cool!

seems reasonable -- but agree w/ vrk we'll have to take this patch to WebKit 

after creating an account and uploading the patch can you cc myself, vrk, and
eric.carlson?

[DaleCurtis, 2011-11-23 02:12:03]

PTAL for unofficial LGTM. My editor fixed a lot of trailing white space... if
this is undesired I can resubmit w/o white space fixes.

On 2011/11/23 01:41:09, scherkus wrote:
> cool!
> 
> seems reasonable -- but agree w/ vrk we'll have to take this patch to WebKit 
> 
> after creating an account and uploading the patch can you cc myself, vrk, and
> eric.carlson?

[scherkus (not reviewing), 2011-11-23 02:43:21]

http://codereview.chromium.org/8496044/diff/34001/Source/WebCore/html/shadow/...
File Source/WebCore/html/shadow/MediaControlRootElement.cpp (right):

http://codereview.chromium.org/8496044/diff/34001/Source/WebCore/html/shadow/...
Source/WebCore/html/shadow/MediaControlRootElement.cpp:540:
m_timeline->setPosition(m_mediaElement->currentTime());
wk uses 4 space indents

http://codereview.chromium.org/8496044/diff/34001/Source/WebCore/html/shadow/...
Source/WebCore/html/shadow/MediaControlRootElement.cpp:540:
m_timeline->setPosition(m_mediaElement->currentTime());
why do we need to reset the position?

http://codereview.chromium.org/8496044/diff/34001/Source/WebCore/html/shadow/...
File Source/WebCore/html/shadow/MediaControlRootElementChromium.cpp (right):

http://codereview.chromium.org/8496044/diff/34001/Source/WebCore/html/shadow/...
Source/WebCore/html/shadow/MediaControlRootElementChromium.cpp:298:
m_timeline->setPosition(m_mediaElement->currentTime());
ditto

http://codereview.chromium.org/8496044/diff/34001/Source/WebCore/html/shadow/...
File Source/WebCore/html/shadow/MediaControlRootElementChromium.h (right):

http://codereview.chromium.org/8496044/diff/34001/Source/WebCore/html/shadow/...
Source/WebCore/html/shadow/MediaControlRootElementChromium.h:87: void
bufferingProgressed();
are there other MediaControls subclasses that we need to update?

[DaleCurtis, 2011-11-24 00:03:23]

Added a layout test. Although for some reason it didn't generated the
-expected.txt in the test directory, just the platform files. annacc suggested I
copy over the platform expected for now; which is what I have done.

http://codereview.chromium.org/8496044/diff/34001/Source/WebCore/html/shadow/...
File Source/WebCore/html/shadow/MediaControlRootElement.cpp (right):

http://codereview.chromium.org/8496044/diff/34001/Source/WebCore/html/shadow/...
Source/WebCore/html/shadow/MediaControlRootElement.cpp:540:
m_timeline->setPosition(m_mediaElement->currentTime());
On 2011/11/23 02:43:22, scherkus wrote:
> wk uses 4 space indents

Done.

http://codereview.chromium.org/8496044/diff/34001/Source/WebCore/html/shadow/...
Source/WebCore/html/shadow/MediaControlRootElement.cpp:540:
m_timeline->setPosition(m_mediaElement->currentTime());
On 2011/11/23 02:43:22, scherkus wrote:
> why do we need to reset the position?

It's the simplest (only?) way to ensure a repaint occurs and it's used above in
the pause/play states 

Another option might be to create a new DOM event (or reuse an existing one) and
call the control's defaultEventHandler(). I didn't see any existing pattern or
obvious event choices, so I stuck with setPosition().

http://codereview.chromium.org/8496044/diff/34001/Source/WebCore/html/shadow/...
File Source/WebCore/html/shadow/MediaControlRootElementChromium.cpp (right):

http://codereview.chromium.org/8496044/diff/34001/Source/WebCore/html/shadow/...
Source/WebCore/html/shadow/MediaControlRootElementChromium.cpp:298:
m_timeline->setPosition(m_mediaElement->currentTime());
On 2011/11/23 02:43:22, scherkus wrote:
> ditto

Done.

http://codereview.chromium.org/8496044/diff/34001/Source/WebCore/html/shadow/...
File Source/WebCore/html/shadow/MediaControlRootElementChromium.h (right):

http://codereview.chromium.org/8496044/diff/34001/Source/WebCore/html/shadow/...
Source/WebCore/html/shadow/MediaControlRootElementChromium.h:87: void
bufferingProgressed();
On 2011/11/23 02:43:22, scherkus wrote:
> are there other MediaControls subclasses that we need to update?

Not that I could find using code search.

[scherkus (not reviewing), 2011-11-28 22:21:19]

http://codereview.chromium.org/8496044/diff/37001/LayoutTests/http/tests/medi...
File LayoutTests/http/tests/media/video-buffering-repaints-controls-expected.txt
(right):

http://codereview.chromium.org/8496044/diff/37001/LayoutTests/http/tests/medi...
LayoutTests/http/tests/media/video-buffering-repaints-controls-expected.txt:3:
layer at (0,0) size 800x600
this is likely a chromium-specific expectation, in which case it'll have to get
placed under one of the LayoutTests/platform/chromium* directories

http://codereview.chromium.org/8496044/diff/37001/LayoutTests/http/tests/medi...
File LayoutTests/http/tests/media/video-buffering-repaints-controls.html
(right):

http://codereview.chromium.org/8496044/diff/37001/LayoutTests/http/tests/medi...
LayoutTests/http/tests/media/video-buffering-repaints-controls.html:23:
video.src =
'http://127.0.0.1:8000/media/video-throttled-load.cgi?&name=resources/test.ogv&throttle=20&type=video/ogg'
take a look at other http/tests/media tests where we have a function that
chooses the correct filename and MIME type based on platform support

http://codereview.chromium.org/8496044/diff/37001/LayoutTests/http/tests/medi...
LayoutTests/http/tests/media/video-buffering-repaints-controls.html:29: <p>Test
that media controls repaint correctly during paused states when new data is
buffered.</p>
maybe: s/during paused states/while paused/ ?

[DaleCurtis, 2011-11-28 23:49:39]

http://codereview.chromium.org/8496044/diff/37001/LayoutTests/http/tests/medi...
File LayoutTests/http/tests/media/video-buffering-repaints-controls-expected.txt
(right):

http://codereview.chromium.org/8496044/diff/37001/LayoutTests/http/tests/medi...
LayoutTests/http/tests/media/video-buffering-repaints-controls-expected.txt:3:
layer at (0,0) size 800x600
Ah, so it didn't create the non-platform specific version because there isn't
one? Is it okay to check files I've generated locally into the platform specific
dirs? I thought the bots generated the files and then I included those. 

On 2011/11/28 22:21:19, scherkus wrote:
> this is likely a chromium-specific expectation, in which case it'll have to
get
> placed under one of the LayoutTests/platform/chromium* directories

http://codereview.chromium.org/8496044/diff/37001/LayoutTests/http/tests/medi...
File LayoutTests/http/tests/media/video-buffering-repaints-controls.html
(right):

http://codereview.chromium.org/8496044/diff/37001/LayoutTests/http/tests/medi...
LayoutTests/http/tests/media/video-buffering-repaints-controls.html:23:
video.src =
'http://127.0.0.1:8000/media/video-throttled-load.cgi?&name=resources/test.ogv&throttle=20&type=video/ogg'
Ah, I saw that, but misunderstood what it was doing. I'll fix it.

On 2011/11/28 22:21:19, scherkus wrote:
> take a look at other http/tests/media tests where we have a function that
> chooses the correct filename and MIME type based on platform support

http://codereview.chromium.org/8496044/diff/37001/LayoutTests/http/tests/medi...
LayoutTests/http/tests/media/video-buffering-repaints-controls.html:29: <p>Test
that media controls repaint correctly during paused states when new data is
buffered.</p>
On 2011/11/28 22:21:19, scherkus wrote:
> maybe: s/during paused states/while paused/ ?

I said paused states instead of paused because I was trying to differentiate
between a never played pause state versus a playing->paused state. Happy to drop
it if that's a useless differentiation.

[scherkus (not reviewing), 2011-11-29 02:02:33]

http://codereview.chromium.org/8496044/diff/37001/LayoutTests/http/tests/medi...
File LayoutTests/http/tests/media/video-buffering-repaints-controls-expected.txt
(right):

http://codereview.chromium.org/8496044/diff/37001/LayoutTests/http/tests/medi...
LayoutTests/http/tests/media/video-buffering-repaints-controls-expected.txt:3:
layer at (0,0) size 800x600
My memory is actually a little foggy on the best way to generate results for new
layout tests, especially for image-based tests :|

Might have to get this answered on a webkit bug thread!

On 2011/11/28 23:49:40, DaleCurtis wrote:
> Ah, so it didn't create the non-platform specific version because there isn't
> one? Is it okay to check files I've generated locally into the platform
specific
> dirs? I thought the bots generated the files and then I included those. 
> 
> On 2011/11/28 22:21:19, scherkus wrote:
> > this is likely a chromium-specific expectation, in which case it'll have to
> get
> > placed under one of the LayoutTests/platform/chromium* directories
>

[DaleCurtis, 2011-12-14 01:12:57]

On 2011/11/29 02:02:33, scherkus wrote:
>
http://codereview.chromium.org/8496044/diff/37001/LayoutTests/http/tests/medi...
> File
LayoutTests/http/tests/media/video-buffering-repaints-controls-expected.txt
> (right):
> 
>
http://codereview.chromium.org/8496044/diff/37001/LayoutTests/http/tests/medi...
> LayoutTests/http/tests/media/video-buffering-repaints-controls-expected.txt:3:
> layer at (0,0) size 800x600
> My memory is actually a little foggy on the best way to generate results for
new
> layout tests, especially for image-based tests :|
> 
> Might have to get this answered on a webkit bug thread!
> 
> On 2011/11/28 23:49:40, DaleCurtis wrote:
> > Ah, so it didn't create the non-platform specific version because there
isn't
> > one? Is it okay to check files I've generated locally into the platform
> specific
> > dirs? I thought the bots generated the files and then I included those. 
> > 
> > On 2011/11/28 22:21:19, scherkus wrote:
> > > this is likely a chromium-specific expectation, in which case it'll have
to
> > get
> > > placed under one of the LayoutTests/platform/chromium* directories
> >

Closing now that WebKit bug is open,
https://bugs.webkit.org/show_bug.cgi?id=73957