On Sun, 31 Jul 2011 12:58:48 +0800, Yu-Jie Lin <[email protected]> wrote:
> Updated the patch.
> 
> Correct my poor English and edit doc (including a few lines of current
> example commands).
> Removed useless code.
> Add a constant "centered" for "follow_mouse".
> 
> (I apologize that I didn't notice that I should've checked To address
> in Gmail when I replied.)
> 
> On Sun, Jul 31, 2011 at 01:18, Anton Khirnov <[email protected]> wrote:
> > On Sun, 31 Jul 2011 00:24:00 +0800, Yu-Jie Lin <[email protected]> wrote:
> >> @@ -400,12 +422,45 @@ x11grab_read_packet(AVFormatContext *s1, AVPacket 
> >> *pkt)
> >> +        if (follow_mouse == -1) {
> >> +            // follow the mouse, put it at center of grabbing region
> >> +            if (pointer_x - s->width / 2 != s->x_off)
> >
> > You can get rid of those two if()s completely, the line below evaluates
> > to x_off += 0; when it's false, which doesn't do anything.
> >
> >> +                x_off += pointer_x - s->width / 2 - s->x_off;
> >> +            if (pointer_y - s->height / 2 != s->y_off)
> >> +                y_off += pointer_y - s->height / 2 - s->y_off;
> >> +        } else {
> >> +            // follow the mouse, but only move the grabbing region when 
> >> mouse
> >> +            // reaches within certain pixels to the edge.
> >> +            if (x_off < screen_w - s->width && pointer_x > x_off + 
> >> s->width - follow_mouse) {
> >                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > When is this not true? Shouldn't x_off always be between 0 and
> > screen_w - s->width?
> 
> I really put a lot of useless code into my patch...
> 
> >> @@ -452,6 +507,7 @@ static const AVOption options[] = {
> >>      { "video_size", "A string describing frame size, such as 640x480 or 
> >> hd720.", OFFSET(video_size), FF_OPT_TYPE_STRING, {.str = "vga"}, 0, 0, DEC 
> >> },
> >>      { "framerate", "", OFFSET(framerate), FF_OPT_TYPE_STRING, {.str = 
> >> "ntsc"}, 0, 0, DEC },
> >>      { "draw_mouse", "Draw the mouse pointer.", OFFSET(draw_mouse), 
> >> FF_OPT_TYPE_INT, { 1 }, 0, 1, DEC },
> >> +    { "follow_mouse", "Follow the mouse pointer.", OFFSET(follow_mouse), 
> >> FF_OPT_TYPE_INT, { 0 }, -1, INT_MAX, DEC },
> >
> > You should elaborate on the help text here. Any program using lavf can
> > set private options, so people using it might never see the ffmpeg
> > manual.
> >
> > Note that AVOptions allow for named constants, so you can assign a
> > symbolic name like 'centered' to -1. See e.g. libavcodec/flacenc.c for
> > an example.
> 
> Thanks for the advices, I added "centered" and more description.
> From 99cfe170483005a9b034356a720310de4b8d9ab5 Mon Sep 17 00:00:00 2001
> From: Yu-Jie Lin <[email protected]>
> Date: Sat, 30 Jul 2011 18:46:36 +0800
> Subject: [PATCH] x11grab: add follow_mouse AVOption.
> 
> -follow_mouse centered|PIXELS
>   move grabbing region to where mouse pointer at the center; or
>   only move when pointer reaches within PIXELS to the edge.
> 
> Signed-off-by: Yu-Jie Lin <[email protected]>
> ---
>  doc/ffmpeg.texi       |   21 +++++++++++++---
>  doc/indevs.texi       |   22 +++++++++++++++++-
>  libavdevice/x11grab.c |   61 ++++++++++++++++++++++++++++++++++++++++++++++--
>  3 files changed, 96 insertions(+), 8 deletions(-)
> 

Looks fine now, except for a few long lines which I'll fix myself.
I'll push later today if nobody has other comments.

Thanks for your contribution.

-- 
Anton Khirnov
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel

Reply via email to