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.

Sign up for GitHub

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

Jump to bottom

Added Blur feature #26

Merged
merged 7 commits into from
Aug 27, 2019
Merged

Added Blur feature #26

merged 7 commits into from
Aug 27, 2019

Conversation

kavishgambhir
Copy link
Contributor

ref: #25

@kavishgambhir
Copy link
Contributor Author

Blur function is CPU intensive so I think there should be an upper bound on the radius.

albertusdev
albertusdev suggested changes Aug 22, 2019
View reviewed changes
Copy link
Contributor

@albertusdev albertusdev left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Hey @kavishgambhir ! Thanks for the contribution! This is very nice.
Some minor feedbacks/questions would be:

  • do you have any reason on making new test.jpg file? Instead of making new file, can we reuse the test.png file instead and decode the blurred testcase in png instead? If there's no concern/reasoning around why we should use jpeg, can you change it to png instead?

pkg/service/manipulator.go Outdated Show resolved Hide resolved
pkg/service/manipulator.go Outdated Show resolved Hide resolved
pkg/service/manipulator.go Outdated Show resolved Hide resolved
pkg/service/manipulator.go Outdated Show resolved Hide resolved
pkg/service/manipulator.go Outdated Show resolved Hide resolved
pkg/processor/interface.go Outdated Show resolved Hide resolved
pkg/processor/native/processor_test.go Show resolved Hide resolved
pkg/processor/native/processor_test.go Show resolved Hide resolved
@ajatprabha
Copy link
Member

ajatprabha commented Aug 27, 2019 •
edited
Loading

LGTM for now. Thank you for your contribution @kavishgambhir!
@baskarap @adalberht Can you merge this if it looks okay to you as well.

PS: Adding a TODO for myself to wrap the Process call in a separate goroutine with a termination timeout.

baskarap
baskarap approved these changes Aug 27, 2019
View reviewed changes
Copy link
Member

@baskarap baskarap left a comment

Choose a reason for hiding this comment

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

LGTM

albertusdev
albertusdev approved these changes Aug 27, 2019
View reviewed changes
Copy link
Contributor

@albertusdev albertusdev left a comment •
edited
Loading

Choose a reason for hiding this comment

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

also LGTM

@baskarap baskarap merged commit d1725c3 into gojek:master Aug 27, 2019
@albertusdev albertusdev mentioned this pull request Aug 27, 2019
Closed
@kavishgambhir kavishgambhir deleted the blur branch August 27, 2019 09:27
@kavishgambhir
Copy link
Contributor Author

LGTM for now. Thank you for your contribution @kavishgambhir!
@baskarap @adalberht Can you merge this if it looks okay to you as well.

PS: Adding a TODO for myself to wrap the Process call in a separate goroutine with a termination timeout.

If you don't mind I would like to take up this issue

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

@baskarap baskarap baskarap approved these changes

@ajatprabha ajatprabha ajatprabha approved these changes

@albertusdev albertusdev albertusdev approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@kavishgambhir @ajatprabha @baskarap @albertusdev