Skip to content
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

Add configurable options to improve output.screen node #547

Merged
merged 5 commits into from
Dec 17, 2021

Conversation

leeping-ng
Copy link
Contributor

closes #543

Summary of improvements:

  1. Gives users the option to resize the output window via configs, for situations such as high-res videos on a small laptop screen, where not the entire output window can be seen
  2. The window size can now also be adjusted by clicking and dragging the output window
  3. The top left corner of the output window will now be at the top left corner of the screen by default. It can also be changed in the configs. Previously, the top left corner of the output window would be around the middle of my screen, and I always had to drag the output window up to see the whole thing.
  4. Gives users the option to change the name of the output window, when previously it was fixed as "PeekingDuck"

@leeping-ng leeping-ng requested a review from ongtw December 1, 2021 08:42
@ongtw ongtw added the enhancement New feature or request label Dec 2, 2021
Copy link
Contributor

@ongtw ongtw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Provided some suggestions.

window_height: 720
window_x_coord: 0
window_y_coord: 0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggest OOP-ing the above with a window object:

window: {
    name: "PeekingDuck",
    x: 0,
    y: 0,
    width: 1280,
    height: 720,
}

Facilitates future attribute extension.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking of this at first, but thought it would be clunky when users read our RTD page. I'd have to lump all the descriptions under one Dict type which can look messy with different descriptions. Also there's a mix of int and str types that get lost when all are lumped under Dict.

Example from resize config from input.live - I think this is simple enough to be nested though.
image

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively, I could do it this way, which is a good compromise:

window_name: "PeekingDuck"
window_loc: {
   x: 0,
   y: 0 }
window_size: {
   width: 1280,
   height: 720 }

Copy link
Contributor

@ongtw ongtw Dec 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. The resize (Dict) example also relies on the user to infer the bool and int types, so it actually servers to highlight the point made on lost type info.
  2. The alternative you suggested is a compromise that I am neutral about.
  3. Another suggestion is to remove window_ totally, just have
inputs: [...]
outputs: [...]

name: "PeekingDuck"
x: 0
y: 0
width: 1280
height: 720

and the code will be self.height, self.width, etc.
Since, after all, the screen is the display.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tried the last suggestion, but the keyword name causes an AttributeError: can't set attribute.

In the end I went for the in-between solution, the RTD looks like this:
image

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, let's go with the in-between solution.
Good reminder that name is reserved, for future similar cases, we'll have to use title or label.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

resizeWindow() doesn't work for opencv >= 4.5.x, only applying resize() to the image works. Unfortunately, it is the opposite case for opencv == 4.1.2.30. To avoid discrepancies in behaviour, in this PR, will go with the solution that works for 4.5.x, and will create another PR to upgrade our openCV version (issue #556).

Added a boolean config for resizing window that's consistent with input.live and input.recorded.

"""

def __init__(self, config: Dict[str, Any] = None, **kwargs: Any) -> None:
super().__init__(config, node_path=__name__, **kwargs)
cv2.namedWindow(self.window_name, cv2.WINDOW_NORMAL)
cv2.moveWindow(self.window_name, self.window_x_coord, self.window_y_coord)
cv2.resizeWindow(self.window_name, self.window_width, self.window_height)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In line with the suggested window object above, Node configs will accept a window object and code will reference its attributes via self.window.name, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated accordingly.

peekingduck/pipeline/nodes/output/screen.py Show resolved Hide resolved
@ongtw
Copy link
Contributor

ongtw commented Dec 2, 2021

Tested your feature branch by setting window loc to (10, 10) and size to 720x360. Works correctly with input.recorded but not with input.live.

This is using input.live: window is shifted to (10, 10) but size is larger than 720x360.
cf. my screen is 1440x900. run_config.yml contents in top-right window.
Screenshot 2021-12-02 at 6 16 08 PM_blur

This is using input.recorded: window correctly shifted to (10, 10) and size correctly set to 720x360.
cf. my screen is 1440x900. run_config.yml contents in top-right window.
Screenshot 2021-12-02 at 6 15 30 PM

@leeping-ng
Copy link
Contributor Author

leeping-ng commented Dec 9, 2021

Tested your feature branch by setting window loc to (10, 10) and size to 720x360. Works correctly with input.recorded but not with input.live.

This is using input.live: window is shifted to (10, 10) but size is larger than 720x360. cf. my screen is 1440x900. run_config.yml contents in top-right window.

This is using input.recorded: window correctly shifted to (10, 10) and size correctly set to 720x360. cf. my screen is 1440x900. run_config.yml contents in top-right window.

Looks like the issue is caused by the opencv version. I had no issues when using opencv-python-4.1.2.30, but it didn't resize for both input.recorded and input.live when I upgraded to opencv-python-4.5.1.48. Will dig deeper into this.

@ongtw
Copy link
Contributor

ongtw commented Dec 9, 2021

Looks like the issue is caused by the opencv version. I had no issues when using opencv-python-4.1.2.30, but it didn't resize for both input.recorded and input.live when I upgraded to opencv-python-4.5.1.48. Will dig deeper into this.

opencv for M1 Mac is 4.5.1. Looks like behavioural changes from earlier versions.

@leeping-ng
Copy link
Contributor Author

Looks like the issue is caused by the opencv version. I had no issues when using opencv-python-4.1.2.30, but it didn't resize for both input.recorded and input.live when I upgraded to opencv-python-4.5.1.48. Will dig deeper into this.

opencv for M1 Mac is 4.5.1. Looks like behavioural changes from earlier versions.

Added issue #556 - I'll raise another PR to update our opencv version to 4.5.x, and the solution in this PR will tackle behaviour for 4.5.x and higher.

Copy link
Contributor

@ongtw ongtw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested a minor refactoring.

resized_img = cv2.resize(
inputs["img"], (self.window_size["width"], self.window_size["height"])
)
cv2.imshow(self.window_name, resized_img)
Copy link
Contributor

@ongtw ongtw Dec 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would like to propose streamlining the code segment in lines 55-61 as:

img = inputs["img"]
if self.window_size["do_resizing"]:
    img = cv2.resize(img, (self.window_size["width"], self.window_size["height"]))
cv2.imshow(self.window_name, img)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks better, updated.

@ongtw
Copy link
Contributor

ongtw commented Dec 16, 2021

Tested the following scenario (M1 Mac opencv 4.5.1):

  1. set window resizing = True, width = 720, height = 360
  2. run PKD: 720x360 window appears with recorded input
  3. manually resize PKD's window

The PKD window is now the new size, even though the cv2.resize(...) portion still runs (added print statement to confirm), i.e. the window size is now different from the specified 720x360.

I suppose this is an "acceptable" behavior?

@leeping-ng
Copy link
Contributor Author

Tested the following scenario (M1 Mac opencv 4.5.1):

  1. set window resizing = True, width = 720, height = 360
  2. run PKD: 720x360 window appears with recorded input
  3. manually resize PKD's window

The PKD window is now the new size, even though the cv2.resize(...) portion still runs (added print statement to confirm), i.e. the window size is now different from the specified 720x360.

I suppose this is an "acceptable" behavior?

Yes, I think this is acceptable. To let users know that this may happen (and that it's an option to manually resize), I added to the window_size config description: "The size of the displayed window can also be adjusted by clicking and dragging."

Copy link
Contributor

@ongtw ongtw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updates 👍🏼

@ongtw ongtw merged commit 6527660 into aisingapore:dev Dec 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow output.screen window size to be configured, to deal with very large images
2 participants