[FFmpeg-devel] [PATCH] Screen frame grabbing for Win32 platform (bump)

[Christophe Gisquet]

Hi,
2010/3/19 Ramiro Polla <ramiro.polla at gmail.com>:
>> + ?--enable-win32grab ? ? ? enable Windows grabbing [no]
>
> I'd prefer a name with gdi in it.

Changed.

> And you don't need to add this
> option to configure. There is an option for x11 because it is GPL'd,
> so you need to enable it explicitly. You can also remove more stuff
> below...
[...]
>> + ? ?win32grab
>
> ...here...

Indeed, OK. Removed.

>> +win32_grab_device_indev_deps="win32grab GetDC"
>
> ...the win32grab dependency here...
>
>> +win32_grab_device_indev_extralibs="-lgdi32"
>>
>> ?# protocols
>> ?gopher_protocol_deps="network"
>> @@ -2701,6 +2705,9 @@
>> ?check_func XShmCreateImage -lX11 -lXext &&
>> ?check_func XFixesGetCursorImage -lX11 -lXext -lXfixes
>>
>> +enabled win32grab ? ? ? ? ? ? ? ? ? ? ? &&
>
> ...and here.

That I hope I understood.

>> + at example
>> +ffmpeg -f win32grab -s cif /tmp/out.mpg
>
> no -i?

Indeed, it's just that the default settings (grabbing the whole
desktop) requires no information to be passed through -i. So I was
undecided as to what to put here. I went with 'foobar'

>> +#undef printf
>> + ? ? ? ?printf("Now: %s\n", param);
>
> Hmm...

Removed.

>
>> + ? ? ? ?if (!strncmp(param, "offset=", 7))
>> + ? ? ? ? ? ?sscanf(param+7, "%i,%i", &s->x_off, &s->y_off);
>
> If the user specifies a window, an offset will make it capture past
> the end of the window.

This made me more thorough on the whole window thing. So now:
- The window's name can be anywhere in the filename
- When size is not specified, capture is restricted to the current
window (can be the desktop)
- The above means that with an offset, only a part of the window is
captured (what you intended)

> If the window is not found above, GetDC will return the Desktop. IMO
> it should fail if the window can't be found.

Changed that, it is now checked in the filename parsing.

>> + ? ?if (NULL == s->source_hdc) {
>
> The "s->source_hdc == errorcode" order is preferred, but in this case
> it should be if (!s->source_hdc). Did you run the patch through
> patcheck?

No, I don't follow enough ffmpeg development to have noticed this addition.
Fixed most things, ignoring blatant errors.

>> + ? ?if (ap->width>s->width || ap->height>s->height) {
>
> Spaces could help readability here around > and <.

I find this more readable (better separating top-level conditions),
but ok, will modify.

>
>> + ? ? ? ?av_log(s1, AV_LOG_ERROR, "Can't force %ix%i resolution, aborting.\n",
>> + ? ? ? ? ? ? ? ap->width, ap->height);
>
> The error message should mention that the specified width/height are
> greater than that of the window.

Done. I'm not mentionning again the window dimensions, as it is
printed before in the log.


>> +/**
>> + * Paints a mouse pointer in an Win32 image.
>> + *
>> + * @param image image to paint the mouse pointer to
>> + * @param s context used to retrieve original grabbing rectangle
>> + * ? ? ? ? ?coordinates
>> + * @param x Mouse pointer coordinate
>> + * @param y Mouse pointer coordinate
>> + */

Fixed incorrent doxy comments.

>> + ? ? ? ? ? ? ? ? ? ?WIN32_API_ERROR("Couldn't get icon rectangle");
>> + ? ? ? ? ? ? ? ?}
>> + ? ? ? ? ? ?}
>> +
>> + ? ? ? ? ? ?if (!DrawIcon(s->window_hdc, x, y, icon))
>> + ? ? ? ? ? ? ? ?WIN32_API_ERROR("Couldn't draw icon");
>> + ? ? ? ?}
>> + ? ? ? ?else {
>> + ? ? ? ? ? ?WIN32_API_ERROR("Couldn't get cursor info");
>> + ? ? ? ?}
>> +
>> + ? ? ? ?DestroyIcon(icon);
>> + ? ?}
>> + ? ?else
>> + ? ? ? ?WIN32_API_ERROR("Cursor not showing?");
>> +}
>
> Won't these errors spam the console for every frame in case it always fails?

I see two meanings to this question:
- abort on first error (not worth it, it sometimes fail for no obvious reason)
- print this only once, possibly failing after some number

Not sure what the best is.

>> + ? ?for(;;) {
>> + ? ? ? ?curtime = av_gettime();
>> + ? ? ? ?delay = s->time_frame * av_q2d(s->time_base) - curtime;
>> + ? ? ? ?if (delay <= 0) {
>> + ? ? ? ? ? ?if (delay < INT64_C(-1000000) * av_q2d(s->time_base)) {
>> + ? ? ? ? ? ? ? ?s->time_frame += INT64_C(1000000);
>> + ? ? ? ? ? ?}
>> + ? ? ? ? ? ?break;
>> + ? ? ? ?}
>> + ? ? ? ?Sleep(delay/1000);
>> + ? ?}
>
> You should also support nonblocking reads.

I agree, but:
1) X11 grabbing does the same, suggesting a common solution
2) This common solution imho should be outside the device demuxers anyway
3) I'm not volunteering for coding an extended solution

In my humble opinion, it could be done by something like a grabbing
thread posting grabbed frames to an encoding thread. Maybe
overengineering but all in all, way too complex for the scope of an
almost deprecated grab interface.

Best regards,
Christophe