Added Multithreaded image magic and file write execution.

[deibele1]

Ruby is thread locked so typically you wont see much improvement from multithreading unless ruby is waiting on an external resource. Image Magick and writing to the drive can run while their ruby thread sleeps and the main thread continues for a dramatic performance improvement.

[samnissen]

@deibele1 Thanks for this. It makes a lot of sense, as this stitch is lengthy. However, I think we need to update our tests – not just because they are failing with this change, but because we should also use them to teach people (including me :] ) how best to wait for the file to be written.

$ bundle _1.16.0_ exec rspec spec/watir-screenshot-stitch_spec.rb:25
Run options: include {:locations=>{"./spec/watir-screenshot-stitch_spec.rb"=>[25]}}

Watir::Screenshot
  driving firefox
    saves stitched-together color screenshot (FAILED - 1)

Failures:

  1) Watir::Screenshot driving firefox saves stitched-together color screenshot
     Failure/Error: expect(image.colorspace).not_to match(/gray/i)

       expected "DirectClass Gray Alpha" not to match /gray/i
       Diff:
       @@ -1 +1 @@
       -/gray/i
       +"DirectClass Gray Alpha"

     # ./spec/watir-screenshot-stitch_spec.rb:45:in `block (3 levels) in <top (required)>'

Finished in 11.12 seconds (files took 0.4652 seconds to load)
1 example, 1 failure

Failed examples:

rspec ./spec/watir-screenshot-stitch_spec.rb:25 # Watir::Screenshot driving firefox saves stitched-together color screenshot

[deibele1]

One way make sure the action is completed for the sake of testing would be return the thread and join it before starting your test. I'll push some changes for you to try.

On Fri, May 14, 2021 at 5:10 AM Sam @.***> wrote:

@deibele1 https://github.com/deibele1 Thanks for this. It makes a lot of sense, as this stitch is lengthy. However, I think we need to update our tests – not just because they are failing with this change, but because we should also use them to teach people (including me :] ) how best to wait for the file to be written.

$ bundle 1.16.0 exec rspec spec/watir-screenshot-stitch_spec.rb:25

Run options: include {:locations=>{"./spec/watir-screenshot-stitch_spec.rb"=>[25]}}

Watir::Screenshot

driving firefox

saves stitched-together color screenshot (FAILED - 1)

Failures:

1. Watir::Screenshot driving firefox saves stitched-together color screenshot

   Failure/Error: expect(image.colorspace).not_to match(/gray/i)

   expected "DirectClass Gray Alpha" not to match /gray/i

   Diff:

   @@ -1 +1 @@

   -/gray/i

   +"DirectClass Gray Alpha"

   ./spec/watir-screenshot-stitch_spec.rb:45:in `block (3 levels) in <top (required)>'

Finished in 11.12 seconds (files took 0.4652 seconds to load)

1 example, 1 failure

Failed examples:

rspec ./spec/watir-screenshot-stitch_spec.rb:25 # Watir::Screenshot driving firefox saves stitched-together color screenshot

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/samnissen/watir-screenshot-stitch/pull/64#issuecomment-841119346, or unsubscribe https://github.com/notifications/unsubscribe-auth/AOST6N2FWPL4V2PFQ5QXEHLTNTSHFANCNFSM44HVPATA .

[deibele1]

Another approach would be to let a parameter determine whether to run the stitch concurrently or synchronously.

On Fri, May 14, 2021 at 9:45 AM Aaron Deibele @.***> wrote:

One way make sure the action is completed for the sake of testing would be return the thread and join it before starting your test. I'll push some changes for you to try.

On Fri, May 14, 2021 at 5:10 AM Sam @.***> wrote:

@deibele1 https://github.com/deibele1 Thanks for this. It makes a lot of sense, as this stitch is lengthy. However, I think we need to update our tests – not just because they are failing with this change, but because we should also use them to teach people (including me :] ) how best to wait for the file to be written.

$ bundle 1.16.0 exec rspec spec/watir-screenshot-stitch_spec.rb:25

Run options: include {:locations=>{"./spec/watir-screenshot-stitch_spec.rb"=>[25]}}

Watir::Screenshot

driving firefox

saves stitched-together color screenshot (FAILED - 1)

Failures:

1. Watir::Screenshot driving firefox saves stitched-together color screenshot

   Failure/Error: expect(image.colorspace).not_to match(/gray/i)

   expected "DirectClass Gray Alpha" not to match /gray/i

   Diff:

   @@ -1 +1 @@

   -/gray/i

   +"DirectClass Gray Alpha"

   ./spec/watir-screenshot-stitch_spec.rb:45:in `block (3 levels) in <top (required)>'

Finished in 11.12 seconds (files took 0.4652 seconds to load)

1 example, 1 failure

Failed examples:

rspec ./spec/watir-screenshot-stitch_spec.rb:25 # Watir::Screenshot driving firefox saves stitched-together color screenshot

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/samnissen/watir-screenshot-stitch/pull/64#issuecomment-841119346, or unsubscribe https://github.com/notifications/unsubscribe-auth/AOST6N2FWPL4V2PFQ5QXEHLTNTSHFANCNFSM44HVPATA .

[samnissen]

@deibele1 I still like this, but what the test changes show me is that this would break existing implementation, which I'm not in favor of. Could we break out the threading via a default-false option, like

opts = { thread_image_stitching: true }
thread = @browser.screenshot.save_stitch(file_path, opts)
# -or-
@browser.screenshot.save_stitch(file_path, {}) #=> no thread returned

Internally I'm imagining tidying up save_stitch a bit in a way that might allow us to reuse the threading in other places one day:

def save_stitch(path, opts = {})
  return @browser.screenshot.save(path) if base64_capable?

  @options = opts
  @path = path

  calculate_dimensions
  return self.save(@path) if (one_shot? || bug_shot?)

  build_canvas
  gather_slices

  if opts[: thread_image_stitching]
    return thread { do stitching }
  else
    do_stitching
  end
end

# ...
def thread(&block)
  stitch = Thread.new do
    isolator.synchronize do
      block.call
    end
  end
  at_exit { stitch.join }
  stitch
end

def do_stitching
  stitch_together
  @combined_screenshot.write @path
end

Let me know what you think

[deibele1]

It looks like a good approach. I think you missed and underscore in "return thread { do stitching }" and I would name the thread method something like exclusive_thread to better distinguish it from Thread.

[samnissen]

This looks good to me @deibele1, though it is definitely far outside my area of expertise. @sandeepnagra Can you please also review?

[sandeepnagra]

This looks good to me @deibele1, though it is definitely far outside my area of expertise. @sandeepnagra Can you please also review?

Yes, I can take a look at it later today.