Skip to content
This repository has been archived by the owner on Dec 24, 2023. It is now read-only.

馃摝 New Feature: Colors to black functionality #45

Closed
wants to merge 1 commit into from

Conversation

mikesalvia
Copy link

This PR allows for users to switch color in the collected image into black. This was very helpful in verifying "painted on" captions in ExoPlayer and AVPlayer for Android and iOS respectively.

鈿狅笍 Probably best that this is pulled and taken for a test drive.

@mikesalvia
Copy link
Author

@wswebcreation will look into the failure to see why this is happening and see about some unit tests

Copy link
Owner

@wswebcreation wswebcreation left a comment

Choose a reason for hiding this comment

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

Hi @mikesalvia

Thanks for this PR.

Can you also provide some examples in your PR of cases where you used this? This would make it easier to understand for people and those can be use for updating the readme. Do you also have time to look at the comments? Last but not least, can you also update the docs?

@@ -21,11 +21,13 @@ export interface AfterScreenshotOptions {
// The file name options object
fileName: ScreenshotFileNameOptions;
// Elements that need to be hidden (visibility: hidden) before saving a screenshot
hideElements: (HTMLElement | HTMLElement[])[];
hideElements: HTMLElement[];
Copy link
Owner

Choose a reason for hiding this comment

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

Why did you change the interface here? hideElements and removeElements will always be an array that hold or single elements, or a combination of single elements and an array of elements (like [$('something'), $$('something-else')])

@@ -32,7 +32,8 @@
"dependencies": {
"canvas": "^2.6.0",
"chalk": "^2.4.2",
"fs-extra": "^7.0.1"
"fs-extra": "^7.0.1",
"gm": "^1.23.1"
Copy link
Owner

Choose a reason for hiding this comment

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

I try to prevent using non standard environment modules like imagemagick and graphicsmagick. Not every user/customer has this standard installed in their CI pipeline which makes this break in pipelines.

ResembleJS also provides a ignoreColors option, doesn't that work?

Copy link
Author

Choose a reason for hiding this comment

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

I will have to take a look and I will look at commments soon!

Copy link
Author

Choose a reason for hiding this comment

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

OK so ignoreColors does not work for me, that basically will compare at grayscale.

The usage of this solves is reading "painted on" captions from a video frame where the video frame may have slight differences.

Our app is a testbed for our video player, in that player, there are 608 captions painted on, over video. In order to validate them, we need to extract captions. However, since there are small changes to the background image even ignoring colors doesn't help. What this does is take any color and floor it to black (not grays), leaving a pure black image with white text to compare.

Copy link
Author

Choose a reason for hiding this comment

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

Also, these libraries are only looked for and used if this option is enabled... I understand non-standard libraries but if you use this option you will need to install the required components.

@mikesalvia
Copy link
Author

Screen Shot 2019-11-04 at 1 44 09 PM
This is what the feature essentially accomplishes.

@mikesalvia
Copy link
Author

@wswebcreation Wanted to see your thoughts on this feature. I have realized it will rely on GraphicsMagick if installed but fails gracefully if it is not installed.

@wswebcreation
Copy link
Owner

Hi @mikesalvia

I've been thinking about this feature and I do believe this is a really great feature. Still I wonder if this is needs to be done with other libs than ResembleJS. Secondly this is a really unique case (IMHO, sorry) for which we add some extra dependencies (even though you made it fail gracefully if it is not installed).

By adding GraphicsMagick we open a complete new door in doing the comparison, which also increases the change in getting more questions from people on how to use this, if it supports more options and so on.

My proposal would be the following:

  1. Or add this feature in ResembleJS (I'm using a fork) and keep the extra dependencies to 0
  2. Open the door for using GraphicsMagick for comparison, but that means we need to have an extra maintainer for this module because I don't have the time to also support that. The extra maintainer needs to take over all GraphicsMagick releated questions / issues in all modules that use this core and also make sure all docs are up to date, explain what you can do with it and so on

What do you think

@wswebcreation wswebcreation self-assigned this Dec 2, 2019
@wswebcreation
Copy link
Owner

Going to close it due to inactivity

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants