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

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(-)

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

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