rendering_helper - Schedule RenderContent() based on VSync.

rendering_helper - Schedule RenderContent() based on VSync.

When we schedule the RenderContent() faster than the VSync signals.
The program will be blocked at SwapBuffer() and affect the performance of
both video decoding and rendering.

Now we change the code to schedule the RenderContent() base on the
VSync signals so that we won't be blocked at SwapBuffer().

BUG=415901
TEST=Run the vda_unittest on link and peach_pit

Committed: https://crrev.com/15d0c0042f6e84a036908b3912bc97864420215f
Cr-Commit-Position: refs/heads/master@{#300654}

[Owen Lin, 2014-09-24 03:54:34]

owenlin@chromium.org changed reviewers:
+ posciak@chromium.org, wuchengli@chromium.org

[Owen Lin, 2014-09-24 03:54:34]

PTAL. Thanks.

[Owen Lin, 2014-10-02 08:23:45]

owenlin@chromium.org changed reviewers:
+ djkurtz@google.com, piman@chromium.org

[Owen Lin, 2014-10-02 08:23:46]

Hi Antoine and Daniel,

I need your help again. When we try to render the video frame at 60 fps or
higher. (More precisely we call the SwapBuffer every 16 ms). The rendering
became slow (may take 10+ ms). We think it make sense since we need to wait util
VSYNC to release the buffer. 

However, what we observed is the decoding become slow at the same time. I have
discussed this issue with Daniel and he has a theory about it. Both VAAPI and GL
calls send to the X server from the same X client (vda_unittest). And the X will
stop accepting new command before SwapBuffer is done. So decoding get affected.
I think that make sense and will try to verify it.

My question is what is the correct way to prevent this. Use the VSyncProvider so
that we can send the SwapBuffer on the correct timing? Or use some other sync
object to wait.

Thank you for the help.

[piman, 2014-10-03 03:18:19]

On Thu, Oct 2, 2014 at 1:23 AM, <owenlin@chromium.org> wrote:

> Hi Antoine and Daniel,
>
> I need your help again. When we try to render the video frame at 60 fps or
> higher. (More precisely we call the SwapBuffer every 16 ms). The rendering
> became slow (may take 10+ ms). We think it make sense since we need to
> wait util
> VSYNC to release the buffer.
>
> However, what we observed is the decoding become slow at the same time. I
> have
> discussed this issue with Daniel and he has a theory about it. Both VAAPI
> and GL
> calls send to the X server from the same X client (vda_unittest). And the
> X will
> stop accepting new command before SwapBuffer is done. So decoding get
> affected.
> I think that make sense and will try to verify it.
>
> My question is what is the correct way to prevent this. Use the
> VSyncProvider so
> that we can send the SwapBuffer on the correct timing? Or use some other
> sync
> object to wait.
>
> Thank you for the help.
>

Right, when you swap too fast, the driver applies back pressure, by having
SwapBuffers block, and that happens in X iirc.

You can use VSyncProvider to get the frequency at which you can swap (this
would be similar to what Chrome does), or use another mechanism such as
sync objects (via GLFence) to make sure you're not pushing too many frames
at a time (e.g. create a GLFence after the swap, don't swap again until the
fence's HasCompleted() returns true).


>
> https://chromiumcodereview.appspot.com/590403004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Owen Lin, 2014-10-16 06:01:41]

Updated to use VSync. PTAL. Thanks.

[piman, 2014-10-16 21:13:29]

https://codereview.chromium.org/590403004/diff/20001/content/common/gpu/media...
File content/common/gpu/media/rendering_helper.cc (right):

https://codereview.chromium.org/590403004/diff/20001/content/common/gpu/media...
content/common/gpu/media/rendering_helper.cc:306:
&RenderingHelper::UpdateVSyncParameters, base::Unretained(this), done));
Can you document why Unretained is safe?

https://codereview.chromium.org/590403004/diff/20001/content/common/gpu/media...
content/common/gpu/media/rendering_helper.cc:512: base::Unretained(this),
Can you document why Unretained is safe?

https://codereview.chromium.org/590403004/diff/20001/content/common/gpu/media...
content/common/gpu/media/rendering_helper.cc:619: RenderedVideo* video =
&videos_[i];
nit: for (RenderedVideo& video : videos_) {

https://codereview.chromium.org/590403004/diff/20001/content/common/gpu/media...
content/common/gpu/media/rendering_helper.cc:635: // |scheuled_render_time_|.
nit: typo scheduled_render_time_

https://codereview.chromium.org/590403004/diff/20001/content/common/gpu/media...
content/common/gpu/media/rendering_helper.cc:641: vsync_timebase_ +=
vsync_interval_;
vsync_timebase_ may not be close to the current time, i.e. this loop could run
for a long time. vsync_timebase_ gets reset every frame when
UpdateVSyncParameters gets called.

How about:
int64 intervals = (target - vsync_timebase_) / vsync_intervals_;
target = vsync_timebase_ + intervals * vsync_interval_;
?

[Owen Lin, 2014-10-20 08:48:42]

https://codereview.chromium.org/590403004/diff/20001/content/common/gpu/media...
File content/common/gpu/media/rendering_helper.cc (right):

https://codereview.chromium.org/590403004/diff/20001/content/common/gpu/media...
content/common/gpu/media/rendering_helper.cc:306:
&RenderingHelper::UpdateVSyncParameters, base::Unretained(this), done));
On 2014/10/16 21:13:29, piman (Very slow to review) wrote:
> Can you document why Unretained is safe?

Done.

https://codereview.chromium.org/590403004/diff/20001/content/common/gpu/media...
content/common/gpu/media/rendering_helper.cc:512: base::Unretained(this),
On 2014/10/16 21:13:28, piman (Very slow to review) wrote:
> Can you document why Unretained is safe?

Done.

https://codereview.chromium.org/590403004/diff/20001/content/common/gpu/media...
content/common/gpu/media/rendering_helper.cc:619: RenderedVideo* video =
&videos_[i];
On 2014/10/16 21:13:28, piman (Very slow to review) wrote:
> nit: for (RenderedVideo& video : videos_) {

Done. Thanks

https://codereview.chromium.org/590403004/diff/20001/content/common/gpu/media...
content/common/gpu/media/rendering_helper.cc:635: // |scheuled_render_time_|.
On 2014/10/16 21:13:28, piman (Very slow to review) wrote:
> nit: typo scheduled_render_time_

Done.

https://codereview.chromium.org/590403004/diff/20001/content/common/gpu/media...
content/common/gpu/media/rendering_helper.cc:641: vsync_timebase_ +=
vsync_interval_;
On 2014/10/16 21:13:28, piman (Very slow to review) wrote:
> vsync_timebase_ may not be close to the current time, i.e. this loop could run
> for a long time. vsync_timebase_ gets reset every frame when
> UpdateVSyncParameters gets called.
> 
> How about:
> int64 intervals = (target - vsync_timebase_) / vsync_intervals_;
> target = vsync_timebase_ + intervals * vsync_interval_;
> ?

Thanks.

[piman, 2014-10-21 19:37:18]

lgtm

[Owen Lin, 2014-10-22 03:54:46]

The CQ bit was checked by owenlin@chromium.org

[commit-bot: I haz the power, 2014-10-22 03:55:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/590403004/40001

[commit-bot: I haz the power, 2014-10-22 03:58:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-22 03:58:10]

Try jobs failed on following builders:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/71818)
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
  android_clang_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Owen Lin, 2014-10-22 07:56:31]

The CQ bit was checked by owenlin@chromium.org

[commit-bot: I haz the power, 2014-10-22 07:57:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/590403004/60001

[commit-bot: I haz the power, 2014-10-22 08:54:07]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2014-10-22 08:55:08]

Patchset 4 (id:??) landed as
https://crrev.com/15d0c0042f6e84a036908b3912bc97864420215f
Cr-Commit-Position: refs/heads/master@{#300654}