New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introducing Burst mode #705

Merged
merged 5 commits into from Oct 19, 2018

Conversation

Projects
None yet
3 participants
@kblok
Owner

kblok commented Oct 17, 2018

PerformScreenshot can make up to 4 WebSocket calls before performing the screenshot:

  • Target.activateTarget
  • Page.getLayoutMetrics
  • Emulation.setDeviceMetricsOverride
  • Emulation.setDefaultBackgroundColorOverride

Then it calls Page.captureScreenshot and then it might need to call Emulation.setDefaultBackgroundColorOverride and SetViewportAsync.

This hurt the performance when trying to perform screenshots in "burst mode".
With "burst mode" I mean taking a series of screenshots to the same page during a certain period of time.

With this new BurstMode we would perform those first five calls only on the first screenshot. Then we would call only Page.captureScreenshot. This will make this burst process at least 2.5x times faster.

After screenshots are taken, the user will need to call SetBurstModeOff explicitelly. So we run Emulation.setDefaultBackgroundColorOverride and SetViewportAsync.

closes #703

kblok added some commits Oct 17, 2018

Ops

@kblok kblok requested a review from Meir017 Oct 17, 2018

@Meir017

This comment has been minimized.

Collaborator

Meir017 commented Oct 18, 2018

@kblok I'm not completely sure how this burst feature works, can you briefly explain it?

@kblok

This comment has been minimized.

Owner

kblok commented Oct 18, 2018

@Meir017 yes, sorry. I planned to be a little bit more detailed, and I forgot to 😂

I'll write some explanation tonight.

_screenshotBurstModeOn = false;
if (_screenshotBurstModeOptions != null)
{
await ResetBackgroundColorAndViewport(_screenshotBurstModeOptions);

This comment has been minimized.

@Meir017

Meir017 Oct 18, 2018

Collaborator

use ConfigureAwait or just return ResetBackgroundColorAndViewport(_screenshotBurstModeOptions)
and after the if statement return Task.Completed

@@ -43,6 +43,9 @@ public class Page : IDisposable
private readonly Dictionary<string, Worker> _workers;
private readonly ILogger _logger;
private bool _ensureNewDocumentNavigation;
private PageGetLayoutMetricsResponse _metrics;

This comment has been minimized.

@Meir017

Meir017 Oct 18, 2018

Collaborator

I think this should be renamed to _burstModeMetrics or similar

Suggested change Beta
private PageGetLayoutMetricsResponse _metrics;
private PageGetLayoutMetricsResponse _burstModeMetrics;
private async Task ResetBackgroundColorAndViewport(ScreenshotOptions options)
{
var tasks = new List<Task>();

This comment has been minimized.

@Meir017

Meir017 Oct 18, 2018

Collaborator

maybe create two task variable with a Task.Completed value and then call

var omitBackgroundTask = options?.OmitBackground == true ? 
    Client.SendAsync("Emulation.setDefaultBackgroundColorOverride") : Task.Completed;
var setViewPortTask = (options?.FullPage == true && Viewport != null) ?
    SetViewportAsync(Viewport) : Task.Completed;
return Task.WhenAll(omitBackgroundTask, setViewPortTask);
@kblok

This comment has been minimized.

Owner

kblok commented Oct 18, 2018

Description updated @Meir017

@Meir017

This comment has been minimized.

Collaborator

Meir017 commented Oct 18, 2018

just a few changes

kblok added some commits Oct 19, 2018

cr
cr
@kblok

This comment has been minimized.

Owner

kblok commented Oct 19, 2018

done @Meir017

@Meir017 Meir017 merged commit f764ff3 into master Oct 19, 2018

2 checks passed

CodeFactor No issues found.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@Meir017 Meir017 deleted the v1.9/burst-mode branch Oct 19, 2018

@brightertools

This comment has been minimized.

brightertools commented Oct 19, 2018

Hi,
Will we be able to set the duration and interval in burst mode?
Thanks

@Meir017

This comment has been minimized.

Collaborator

Meir017 commented Oct 19, 2018

You can achieve this using a loop or similar, I don't think this behavior should be baked in

@brightertools

This comment has been minimized.

brightertools commented Oct 19, 2018

Oh I see, I get it now..
We switch bust mode on, then take the screenshots on our duration/interval, then switch it off.
👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment