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

Introduced copy text from image feature on macOS #16508

Merged
merged 10 commits into from
Jan 13, 2023
Merged

Conversation

simonhong
Copy link
Member

@simonhong simonhong commented Jan 3, 2023

fix brave/brave-browser#27513

"Copy Text From Image" entry is added to render view's context menu.
It gets bitmap data from renderer via LocalFrame::GetImage() mojom interface,
and then passed it to TextRecognitionDialog to extract and show text from the image.

Screen_Recording_2023-01-05_at_9_31_27_AM_AdobeExpress

Newly added context meny entry
Screenshot 2023-01-05 at 9 09 26 AM

Dialog shows the copied text from image. Vertical scroll bar can be shown based on the contents.
Screenshot 2023-01-05 at 9 09 38 AM
Screenshot 2023-01-05 at 9 15 50 AM
Screenshot 2023-01-05 at 9 24 57 AM

When text recognition is in-progress:
Screenshot 2023-01-05 at 9 17 55 AM

When text recognition is failed:
Screenshot 2023-01-06 at 9 51 54 AM

Resolves

Submitter Checklist:

  • I confirm that no security/privacy review is needed, or that I have requested one
  • There is a ticket for my issue
  • Used Github auto-closing keywords in the PR description above
  • Wrote a good PR/commit description
  • Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
  • Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
  • Checked the PR locally:
    • npm run test -- brave_browser_tests, npm run test -- brave_unit_tests wiki
    • npm run lint, npm run presubmit wiki, npm run gn_check, npm run tslint
  • Ran git rebase master (if needed)

Reviewer Checklist:

  • A security review is not needed, or a link to one is included in the PR description
  • New files have MPL-2.0 license header
  • Adequate test coverage exists to prevent regressions
  • Major classes, functions and non-trivial code blocks are well-commented
  • Changes in component dependencies are properly reflected in gn
  • Code follows the style guide
  • Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

TextRecognitionBrowserTest.TextRecognitionTest

  1. Search any image that contains text from search.brave.com
  2. Choose Copy Text From Image item from context menu
  3. Check dialog is shown with text
  4. Check extracted text is copied to clipboard

@simonhong simonhong self-assigned this Jan 3, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Jan 3, 2023

⚠️ PR head is an unsigned commit
commit: 6c7ad235ded4fbbb787243620a541fd670fea01a
reason: unsigned
Please follow the handbook to configure commit signing
cc: @simonhong

@github-actions github-actions bot added the potential-layer-violation-fixes This PR touches a BUILD.gn file with check_includes=false label Jan 3, 2023
@simonhong simonhong force-pushed the text_recognition branch 2 times, most recently from 8e10b4b to d951d34 Compare January 4, 2023 15:32
@simonhong simonhong marked this pull request as ready for review January 5, 2023 00:46
@simonhong simonhong requested review from a team as code owners January 5, 2023 00:46
@simonhong
Copy link
Member Author

@diracdeltas Should I have security review for this PR?
This PR extracts text from bitmap image came from renderer by using Vision framework from macOS SDK.
VNImageRequestHandler - https://developer.apple.com/documentation/vision/vnimagerequesthandler?language=objc

@simonhong
Copy link
Member Author

simonhong commented Jan 5, 2023

@rmcfadden3 Can you review the newly added sentences?
As you can see in the above description, we have four new strings.
Copy Text From Image menu entry
Copying text from image... when text recognition is in progress.
Text copied from image when text recognition is completed.
Copy text failed when text recognition is failed.

@diracdeltas
Copy link
Member

This PR extracts text from bitmap image came from renderer by using Vision framework from macOS SDK.

thanks for flagging. is this framework local-only? (AKA no network requests are generated in language detection and text extraction). if so it should be fine.

if not we should maybe disable in Tor windows

also do we need any licensing changes to use the framework? i assume not.

@simonhong
Copy link
Member Author

simonhong commented Jan 5, 2023

This PR extracts text from bitmap image came from renderer by using Vision framework from macOS SDK.

thanks for flagging. is this framework local-only? (AKA no network requests are generated in language detection and text extraction). if so it should be fine.

According to the docs, processing happens on user's device. - https://developer.apple.com/documentation/vision/recognizing_text_in_images?language=objc

One of Vision’s many powerful features is its ability to detect and recognize multilanguage text in images.
You can use this functionality in your own apps to handle both real-time and offline use cases.
In all cases, all of Vision’s processing happens on the user’s device to enhance performance and user privacy.

if not we should maybe disable in Tor windows

also do we need any licensing changes to use the framework? i assume not.

I think we don't need another licensing for builtin SDK cc @fmarier

@diracdeltas
Copy link
Member

no concerns from me then!

@fmarier
Copy link
Member

fmarier commented Jan 5, 2023

I think we don't need another licensing for builtin SDK

If it's an OS feature provided by Apple, then I don't think so either.

@rmcfadden3
Copy link

@rmcfadden3 Can you review the newly added sentences? As you can see in the above description, we have four new strings. Copy Text From Image menu entry Copying text from image... when text recognition is in progress. Text copied from image when text recognition is completed. Copy text failed when text recognition is failed.

@simonhong — all of your text looks great. However, I would slightly edit the last option to make it grammatically correct:

  • Text copy failed

@simonhong
Copy link
Member Author

@rmcfadden3 Can you review the newly added sentences? As you can see in the above description, we have four new strings. Copy Text From Image menu entry Copying text from image... when text recognition is in progress. Text copied from image when text recognition is completed. Copy text failed when text recognition is failed.

@simonhong — all of your text looks great. However, I would slightly edit the last option to make it grammatically correct:

  • Text copy failed

Updated. Thanks!

@bsclifton
Copy link
Member

Functionality works great! 😄 This is really cool - will check out the code next

for (const auto& t : text) {
unified_string += t;
unified_string += '\n';
}
Copy link
Member

@bsclifton bsclifton Jan 9, 2023

Choose a reason for hiding this comment

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

The above is fine 😄 But you could use std library:

const char* const delim = "\n";
std::ostringstream unified;
std::copy(text.begin(), text.end(),
           std::ostream_iterator<std::string>(unified, delim));
std::string unified_string = unified.str();

Need to include <sstream> and maybe some others (lint should warn with include what you use)

Copy link
Member

Choose a reason for hiding this comment

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

Pushed a quick update with this - 1d611fe 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

Inspired from your changes - I found base::JoinString() api!

Copy link
Contributor

Choose a reason for hiding this comment

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

Sometimes I don't understand why we don't have basic operation like join string in std 😭

Copy link
Member

@bsclifton bsclifton Jan 10, 2023

Choose a reason for hiding this comment

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

Nice find! 😃 And yes - I completely agree, @sangwoo108 😄

Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

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

LGTM++

@github-actions
Copy link
Contributor

github-actions bot commented Jan 9, 2023

⚠️ PR head is an unsigned commit
commit: 1d611fe
reason: unsigned
Please follow the handbook to configure commit signing
cc: @bsclifton

@github-actions
Copy link
Contributor

github-actions bot commented Jan 9, 2023

⚠️ PR head is an unsigned commit
commit: 09475f3013a0dcf9dde35a4ad063d21158314a6d
reason: unsigned
Please follow the handbook to configure commit signing
cc: @simonhong

1 similar comment
@github-actions
Copy link
Contributor

github-actions bot commented Jan 9, 2023

⚠️ PR head is an unsigned commit
commit: 09475f3013a0dcf9dde35a4ad063d21158314a6d
reason: unsigned
Please follow the handbook to configure commit signing
cc: @simonhong

@github-actions
Copy link
Contributor

⚠️ PR head is an unsigned commit
commit: bcec82dbe2e3ff4a17ed3b6b17d7d2b69849c4f0
reason: unsigned
Please follow the handbook to configure commit signing
cc: @simonhong

@github-actions
Copy link
Contributor

⚠️ PR head is an unsigned commit
commit: 5594934c17fc803612afa8e5a5ebece6656f8c27
reason: unsigned
Please follow the handbook to configure commit signing
cc: @simonhong

2 similar comments
@github-actions
Copy link
Contributor

⚠️ PR head is an unsigned commit
commit: 5594934c17fc803612afa8e5a5ebece6656f8c27
reason: unsigned
Please follow the handbook to configure commit signing
cc: @simonhong

@github-actions
Copy link
Contributor

⚠️ PR head is an unsigned commit
commit: 5594934c17fc803612afa8e5a5ebece6656f8c27
reason: unsigned
Please follow the handbook to configure commit signing
cc: @simonhong

@github-actions
Copy link
Contributor

⚠️ PR head is an unsigned commit
commit: 758dfb98d6cc24703275f8fa00781ee7bf3e0c79
reason: unsigned
Please follow the handbook to configure commit signing
cc: @simonhong

@github-actions
Copy link
Contributor

⚠️ PR head is an unsigned commit
commit: 9e19f2e4c26994f1dc776e1f3106c2d30bf37303
reason: unsigned
Please follow the handbook to configure commit signing
cc: @simonhong

1 similar comment
@github-actions
Copy link
Contributor

⚠️ PR head is an unsigned commit
commit: 9e19f2e4c26994f1dc776e1f3106c2d30bf37303
reason: unsigned
Please follow the handbook to configure commit signing
cc: @simonhong

@simonhong
Copy link
Member Author

TextRecognitionBrowserTest.TextRecognitionTest browser test is added. PTAL again. Thanks.


#define TEXT_RECOGNITION_BROWSER_EXPORT

#endif
Copy link
Member

@goodov goodov Jan 12, 2023

Choose a reason for hiding this comment

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

please use base/component_export.h instead along with COMPONENT_EXPORT(TEXT_RECOGNITION...) macro.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

"//brave/browser/ui",
"//brave/components/constants",
"//chrome/browser",
"//chrome/browser/devtools:test_support",
"//chrome/browser/profiles:profile",
"//chrome/browser/ui",
"//chrome/test:test_support_ui",
"//chrome/test:unit_tests",
Copy link
Member

Choose a reason for hiding this comment

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

with this line you effectively added browser_tests->unit_tests dependency.
you should definitely not do that. maybe you only need to depend on gtest?

Copy link
Member Author

Choose a reason for hiding this comment

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

I picked wrong target for chrome/browser/renderer_context_menu/render_view_context_menu_test_util.h among below three candidates. //chrome/test:test_support is picked. Thanks!

ERROR at //brave/browser/ui/text_recognition_browsertest.cc:20:11: Include not allowed.
#include "chrome/browser/renderer_context_menu/render_view_context_menu_test_util.h"
          ^------------------------------------------------------------------------
It is not in any dependency of
  //brave/browser/ui:browser_tests
The include file is in the target(s):
  //chrome/test:interactive_ui_tests
  //chrome/test:test_support
  //chrome/test:unit_tests
at least one of which should somehow be reachable.

@github-actions
Copy link
Contributor

⚠️ PR head is an unsigned commit
commit: e6c43a15f3f4f3d5bf9420cd1378f75c83feeabf
reason: unsigned
Please follow the handbook to configure commit signing
cc: @simonhong

"//skia",
]

defines = [ "IS_TEXT_RECOGNITION_BROWSER_IMPL" ]
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

@simonhong simonhong Jan 13, 2023

Choose a reason for hiding this comment

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

Oh, good to know. Updated with verified commit :) !

@github-actions
Copy link
Contributor

⚠️ PR head is an unsigned commit
commit: e6c43a15f3f4f3d5bf9420cd1378f75c83feeabf
reason: unsigned
Please follow the handbook to configure commit signing
cc: @simonhong

simonhong and others added 7 commits January 13, 2023 19:22
fix brave/brave-browser#27513

"Copy Text From Image" entry is added to render view's context menu.
It gets bitmap data from renderer via LocalFrame::GetImage() mojom interface,
and then passed it to TextRecognitionDialog to extract and show text from that
image.
If renderer is busy or on slow machine, copy image could take some time.
In that situation, user could ask copy text menu multiple times. And then,
browser would get multiple copied image from renderer. When it happens,
use same dialog and show lastly recognized text in dialog instead of
creating multiple dialogs.
Copy link
Member

@goodov goodov left a comment

Choose a reason for hiding this comment

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

lgtm with one more comment.

components/text_recognition/browser/text_recognition.h Outdated Show resolved Hide resolved
@simonhong simonhong merged commit 8704c6f into master Jan 13, 2023
@simonhong simonhong deleted the text_recognition branch January 13, 2023 13:12
@github-actions github-actions bot added this to the 1.49.x - Nightly milestone Jan 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
potential-layer-violation-fixes This PR touches a BUILD.gn file with check_includes=false
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Copy Text from Image to image context menu (macOS)
8 participants