-
-
Notifications
You must be signed in to change notification settings - Fork 10.9k
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
Show text after file pushed #1484
base: dev
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for working on this. I agree that some OSD would be better to report status/actions to the user (like FPS count #274 (comment)).
Because the path of font file is difficult to specify in linux based system. I embed open-sans ttf to prevent loading font file failed.
I'm not fan of embedding the font into the source code (I know that I did that for the icon, but it's ASCII-art readable, not an array of hexadecimal values). I'd prefer a separate font-file. By the way, unless we want a specific font (I think we don't), I think it should even use some default sans-serif font available on the system.
I added some inline comments on the PR.
Also, from a user point of view, I think that it is not very clear that the text is added by scrcpy (that it is not part of the device screen). I'm not sure how to improve that. Maybe one reason is that Android already displays notifications as toasts at the same location.
Events like "file pushed" could maybe printed in the bottom in some (overlay) "status bar" (a bit like in Firefox when we hover a link). Or as notification bubbles in the top-right (a bit like the result of notify-send mytext
on Linux, ideally, they would also appear/disappear gracefully). (These are just things I have in mind, but I'm very bad at UI design 😉)
@@ -454,6 +466,90 @@ update_texture(struct screen *screen, const AVFrame *frame) { | |||
} | |||
} | |||
|
|||
bool | |||
screen_render_text(struct screen *screen) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This opens the font and draws the glyphs on every frame :/
IMO, it should create the texture and render the text the first time, then cache it (to be invalidated on resize though).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree.
app/src/screen.c
Outdated
screen_render_text(struct screen *screen) { | ||
struct render_text_request req; | ||
|
||
// we check this flag outside lock, it may cause that we don't show text immediately. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(but it's racy, you should use an atomic instead)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it will race. But the effect is just that it will delay notice that it needs to show text.
This trick added only because I notice the performance has some degrade. Maybe if we don't create texture every frame, we don't need this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But the effect is just that it will delay notice that it needs to show text.
It's a data race, so in theory the flag write may never be seen by the other thread.
#include <pthread.h>
#include <stdbool.h>
#include <stdio.h>
#include <unistd.h>
static bool keep_running = true;
void *run(void *arg) {
(void) arg;
printf("start\n");
while (keep_running);
printf("end\n");
return NULL;
}
int main(void) {
pthread_t thread;
int ret = pthread_create(&thread, NULL, run, NULL);
if (ret)
return 1;
sleep(1);
keep_running = false;
printf("keep_running = false;\n");
ret = pthread_join(thread, NULL);
if (ret)
return 1;
return 0;
}
$ gcc -pthread ub.c && ./a.out
start
keep_running = false;
end
$ gcc -O3 -pthread ub.c && ./a.out
start
keep_running = false;
// never ends
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, maybe lack a volatile
?
BTW, thank you for your example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, maybe lack a volatile?
Or any synchronization fixing the data race (an atomic would probably be preferable).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, thank you.
I will try to fix these issues in weekend.
app/src/screen.c
Outdated
if (screen->curr_text.time <= 0) { | ||
SDL_free(screen->curr_text.text); | ||
screen->curr_text.text = NULL; | ||
screen->curr_text.time = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The time is handled as the number of frames. But the framerate is (very) variable: it may be as low as 0 fps if the device screen is constant to 90fps (for example) if it changes a lot.
The "toast" delay should be based on real time instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What should happen if time is up but the screen is constant? In current implementation, it will remain on screen until screen changed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, the popup/taost delay should be independant of the device video. It should fade out after a constant delay (whether the device screen is constant or not).
Yes, I also think that embed some binary in code is really ugly. However, as far as I know, there are not an easy and reliable way to get usable font cross platform...
Maybe style like notify-send is a good design? |
af12a56
to
4280d11
Compare
Signed-off-by: Yu-Chen Lin <[email protected]>
Signed-off-by: Yu-Chen Lin <[email protected]>
I have pushed new version, it includes:
Some missing parts:
BTW, windows lacks some dll now, I will fix it later. |
I find that in windows, to make this feature working, we need a lot of dll and it is annoy to use current method to manage a lot of dlls. Thank you. |
7e8a942
to
c0de365
Compare
Hi,
In this series, I add a text framework. User can use this framework to show text on screen.
After it, I use this framework to show file pushed.
imgur
Note:
Because the path of font file is difficult to specify in linux based system. I embed open-sans ttf to prevent loading font file failed.
Thanks.