diff mbox series

[FFmpeg-devel] libavdevice/gdigrab: fix capture window title contain non-ASCII chars

Message ID tencent_A50AB9C1BD78BF2DA8BD11F3FB9F2AED560A@qq.com
State New
Headers show
Series [FFmpeg-devel] libavdevice/gdigrab: fix capture window title contain non-ASCII chars
Related show

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate success Make fate finished

Commit Message

1160386205@qq.com March 20, 2021, 3:32 p.m. UTC
From: He Yang <1160386205@qq.com>

Signed-off-by: He Yang <1160386205@qq.com>
---
 libavdevice/gdigrab.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

Comments

Jan Ekström April 12, 2021, 9:35 p.m. UTC | #1
On Sat, Mar 20, 2021 at 5:34 PM <1160386205@qq.com> wrote:
>
> From: He Yang <1160386205@qq.com>
>
> Signed-off-by: He Yang <1160386205@qq.com>

Sorry for taking a while to respond, and thank you for the
contribution. I have verified that this conversion and FindWindowW
usage indeed fixes issues with non-ASCII window titles.

Before:
[gdigrab @ 000001d1f24b2cc0] Can't find window 'ジャンキーナイトタウンオーケストラ _
すりぃ feat.鏡音レン-sm36109943.mp4 - mpv', aborting.
title=ジャンキーナイトタウンオーケストラ _ すりぃ feat.鏡音レン-sm36109943.mp4 - mpv: I/O error

After:
[gdigrab @ 0000017d298b2cc0] Found window ジャンキーナイトタウンオーケストラ _  すりぃ
feat.鏡音レン-sm36109943.mp4 - mpv, capturing 1920x1080x32 at (0,0)
Input #0, gdigrab, from 'title=ジャンキーナイトタウンオーケストラ _ すりぃ
feat.鏡音レン-sm36109943.mp4 - mpv':

Now, taking things step-by-step, first from the most clear things:
1. FFmpeg utilizes C99 features, but follows the rule that no
declarations should happen after non-declaring code within a
scope/context.
src/libavdevice/gdigrab.c: In function 'gdigrab_read_header':
src/libavdevice/gdigrab.c:249:9: warning: ISO C90 forbids mixed
declarations and code [-Wdeclaration-after-statement]
  249 |         const wchar_t *name_w = NULL;
      |         ^~~~~

-> Basically fixed by moving the new wchar_t as the first thing in the
scope of that if branch.

2. Mismatch between function and the calling code in `const`ness.
Const things are nice, but in this case the function takes in a
non-const pointer.

src/libavdevice/gdigrab.c:250:30: warning: passing argument 2 of
'utf8towchar' from incompatible pointer type
[-Wincompatible-pointer-types]
  250 |         if(utf8towchar(name, &name_w)) {
      |                              ^~~~~~~
      |                              |
      |                              const wchar_t ** {aka const short
unsigned int **}
In file included from src/libavformat/os_support.h:148,
                 from src/libavformat/internal.h:28,
                 from src/libavdevice/gdigrab.c:32:
src/libavutil/wchar_filename.h:27:68: note: expected 'wchar_t **' {aka
'short unsigned int **'} but argument is of type 'const wchar_t **'
{aka 'const short unsigned int **'}
   27 | static inline int utf8towchar(const char *filename_utf8,
wchar_t **filename_w)
      |
~~~~~~~~~~^~~~~~~~~~

-> Fixed by removing the const from the wchar_t pointer.

Thus we move to actual review:

1. The libavutil header should be explicitly #included. That way users
of headers should be more easily find'able.
2. When utf8towchar returns nonzero, ret should probably be set to
AVERROR(errno). That way we are not re-guessing implementation
specifics of the function. (noticed by Martin)
3. Some whitespace would be good between the variable
declarations/setting, doing the conversion and finally the actual
window finding.

As I had to go through these points for the review process, I
basically posted a version with these changes @
https://github.com/jeeb/ffmpeg/commits/gdigrab_unicode_fix . I also
took the liberty of rewording the commit message somewhat. If you
think these changes are acceptable, then unless something new is
noticed, I consider this LGTM.

Best regards,
Jan
Jan Ekström April 13, 2021, 4:42 p.m. UTC | #2
On Tue, Apr 13, 2021 at 12:35 AM Jan Ekström <jeebjp@gmail.com> wrote:
>
> On Sat, Mar 20, 2021 at 5:34 PM <1160386205@qq.com> wrote:
> >
> > From: He Yang <1160386205@qq.com>
> >
> > Signed-off-by: He Yang <1160386205@qq.com>
>
> Sorry for taking a while to respond, and thank you for the
> contribution. I have verified that this conversion and FindWindowW
> usage indeed fixes issues with non-ASCII window titles.
>
> Before:
> [gdigrab @ 000001d1f24b2cc0] Can't find window 'ジャンキーナイトタウンオーケストラ _
> すりぃ feat.鏡音レン-sm36109943.mp4 - mpv', aborting.
> title=ジャンキーナイトタウンオーケストラ _ すりぃ feat.鏡音レン-sm36109943.mp4 - mpv: I/O error
>
> After:
> [gdigrab @ 0000017d298b2cc0] Found window ジャンキーナイトタウンオーケストラ _  すりぃ
> feat.鏡音レン-sm36109943.mp4 - mpv, capturing 1920x1080x32 at (0,0)
> Input #0, gdigrab, from 'title=ジャンキーナイトタウンオーケストラ _ すりぃ
> feat.鏡音レン-sm36109943.mp4 - mpv':
>
> Now, taking things step-by-step, first from the most clear things:
> 1. FFmpeg utilizes C99 features, but follows the rule that no
> declarations should happen after non-declaring code within a
> scope/context.
> src/libavdevice/gdigrab.c: In function 'gdigrab_read_header':
> src/libavdevice/gdigrab.c:249:9: warning: ISO C90 forbids mixed
> declarations and code [-Wdeclaration-after-statement]
>   249 |         const wchar_t *name_w = NULL;
>       |         ^~~~~
>
> -> Basically fixed by moving the new wchar_t as the first thing in the
> scope of that if branch.
>
> 2. Mismatch between function and the calling code in `const`ness.
> Const things are nice, but in this case the function takes in a
> non-const pointer.
>
> src/libavdevice/gdigrab.c:250:30: warning: passing argument 2 of
> 'utf8towchar' from incompatible pointer type
> [-Wincompatible-pointer-types]
>   250 |         if(utf8towchar(name, &name_w)) {
>       |                              ^~~~~~~
>       |                              |
>       |                              const wchar_t ** {aka const short
> unsigned int **}
> In file included from src/libavformat/os_support.h:148,
>                  from src/libavformat/internal.h:28,
>                  from src/libavdevice/gdigrab.c:32:
> src/libavutil/wchar_filename.h:27:68: note: expected 'wchar_t **' {aka
> 'short unsigned int **'} but argument is of type 'const wchar_t **'
> {aka 'const short unsigned int **'}
>    27 | static inline int utf8towchar(const char *filename_utf8,
> wchar_t **filename_w)
>       |
> ~~~~~~~~~~^~~~~~~~~~
>
> -> Fixed by removing the const from the wchar_t pointer.
>
> Thus we move to actual review:
>
> 1. The libavutil header should be explicitly #included. That way users
> of headers should be more easily find'able.
> 2. When utf8towchar returns nonzero, ret should probably be set to
> AVERROR(errno). That way we are not re-guessing implementation
> specifics of the function. (noticed by Martin)
> 3. Some whitespace would be good between the variable
> declarations/setting, doing the conversion and finally the actual
> window finding.
>
> As I had to go through these points for the review process, I
> basically posted a version with these changes @
> https://github.com/jeeb/ffmpeg/commits/gdigrab_unicode_fix . I also
> took the liberty of rewording the commit message somewhat. If you
> think these changes are acceptable, then unless something new is
> noticed, I consider this LGTM.
>
> Best regards,
> Jan

Got an ack for these changes on both IRC and Github, thus applied as
707f9c9f475f612e196876708cdb5ead31f63525 .

Thank you once again for the contribution.

Jan
diff mbox series

Patch

diff --git a/libavdevice/gdigrab.c b/libavdevice/gdigrab.c
index 9b2c55fe90..c2842975ec 100644
--- a/libavdevice/gdigrab.c
+++ b/libavdevice/gdigrab.c
@@ -246,7 +246,17 @@  gdigrab_read_header(AVFormatContext *s1)
 
     if (!strncmp(filename, "title=", 6)) {
         name = filename + 6;
-        hwnd = FindWindow(NULL, name);
+        const wchar_t *name_w = NULL;
+        if(utf8towchar(name, &name_w)) {
+            ret = AVERROR(ENOMEM);
+            goto error;
+        }
+        if(!name_w) {
+            ret = AVERROR(EINVAL);
+            goto error;
+        }
+        hwnd = FindWindowW(NULL, name_w);
+        av_freep(&name_w);
         if (!hwnd) {
             av_log(s1, AV_LOG_ERROR,
                    "Can't find window '%s', aborting.\n", name);