On 26/08/14 14:11, Diego Biurrun wrote:
> On Sun, Aug 24, 2014 at 02:18:22PM +0200, Luca Barbato wrote:
>> Feature matching x11grab.
>
> Matches the x11grab screen capture by features.
>
> IMO this thing should deprecate the x11grab screen capture.
>
>> --- a/configure
>> +++ b/configure
>> @@ -4257,6 +4261,13 @@ require Xext X11/extensions/XShm.h XShmCreateImage
>> -lXext &&
>> require Xfixes X11/extensions/Xfixes.h XFixesGetCursorImage -lXfixes &&
>> { enabled xlib || die "ERROR: Xlib not found"; }
>>
>> +enabled xcbgrab &&
>> +require_pkg_config xcb-composite xcb/composite.h
>> xcb_composite_name_window_pixmap &&
>> +require_pkg_config xcb-event xcb/xcb_event.h xcb_event_get_error_label &&
>> +require_pkg_config xcb-xfixes xcb/xfixes.h xcb_xfixes_get_cursor_image &&
>> +require_pkg_config xcb-shm xcb/shm.h xcb_shm_attach &&
>> +check_header sys/shm.h
>> +
>> enabled vdpau &&
>> check_cpp_condition vdpau/vdpau.h "defined
>> VDP_DECODER_PROFILE_MPEG4_PART2_ASP" ||
>> disable vdpau
>
> Indent like the vdpau block below.
>
> Do you really need to check all those things separately?
I could check for all the libraries and a single header.
> Do configurations exist where one part is available w/o the others?
actually xcb-shm could be made an optional requirement. The rest is
required an might be missing due granular packaging (e.g. Debian and
Ubuntu users need to install the dev version of each of those separately)
> Does it work w/o the shm headers?
Could, the configure doesn't let you right now. should I drop that or
the other?
>> +static const AVOption options[] = {
>> + { "video_size", "A string describing frame size, such as 640x480 or
>> hd720.", OFFSET(video_size), AV_OPT_TYPE_STRING, {.str = "vga"}, 0, 0, D },
>> + { "framerate", "", OFFSET(framerate), AV_OPT_TYPE_STRING, {.str =
>> "ntsc"}, 0, 0, D },
>> + { "draw_mouse", "Draw the mouse pointer.", OFFSET(draw_mouse),
>> AV_OPT_TYPE_INT, { .i64 = 1 }, 0, 1, D },
>> + { "follow_mouse", "Move the grabbing region when the mouse pointer
>> reaches within specified amount of pixels to the edge of region.",
>> + OFFSET(follow_mouse), AV_OPT_TYPE_INT, { .i64 = 0}, FOLLOW_CENTER,
>> INT_MAX, D, "follow_mouse" },
>> + { "centered", "Keep the mouse pointer at the center of grabbing region
>> when following.", 0, AV_OPT_TYPE_CONST, { .i64 = -1 }, INT_MIN, INT_MAX, D,
>> "follow_mouse" },
>> + { "show_region", "Show the grabbing region.", OFFSET(show_region),
>> AV_OPT_TYPE_INT, { .i64 = 0 }, 0, 1, D },
>> + { "region_border", "Set the region border thickness.",
>> OFFSET(region_border), AV_OPT_TYPE_INT, { .i64 = 3 }, 1, 128, D },
>> + { NULL },
>
> spaces inside {}
Ok
>> + iq = xcb_get_image(g->conn, XCB_IMAGE_FORMAT_Z_PIXMAP, drawable,
>> + g->x, g->y, g->width, g->height, ~0);
>> + img = xcb_get_image_reply(g->conn, iq, NULL);
>
> You could merge declaration and initialization here.
I'd rather not.
>
>> + data = xcb_get_image_data(img);
>> + length = xcb_get_image_data_length(img);
>
> align
>
>> +#if HAVE_SYS_SHM_H
>> +static void dealloc_shm(void *unused, uint8_t *data)
>> +{
>> + (void)unused;
>> +
>> + shmdt(data);
>> +}
>
> Is there a warning w/o the void cast?
Probably not, I can remove it.
>> + img = xcb_shm_get_image_reply(g->conn, iq, &e);
>> + if (e)
>> + av_log(s, AV_LOG_ERROR,
>> + "event_error: %s: response_type:%u error_code:%u "
>> + "sequence:%u resource_id:%u minor_code:%u major_code:%u\n",
>> + xcb_event_get_error_label(e->error_code), e->response_type,
>> + e->error_code, e->sequence, e->resource_id, e->minor_code,
>> + e->major_code);
>> +
>> + xcb_shm_detach(g->conn, g->segment);
>> + xcb_flush(g->conn);
>> +
>> + if (!img) {
>> + shmctl(id, IPC_RMID, 0);
>> + return AVERROR(ENOMEM);
>> + }
>
> Doing something before the out-of-memory check looks very odd and error-prone.
Actually the error is should be EACCES, thanks for spotting.
>
>> + data = shmat(id, NULL, 0);
>> + shmctl(id, IPC_RMID, 0);
>> +
>> + if ((intptr_t)data == -1)
>> + return AVERROR(ENOMEM);
>
> IIUC shmat can return all sorts of errors and sets errno accordingly.
I can wrap errno
[EACCES] The calling process has no permission to access
this shared memory segment.
[EINVAL] shmid is not a valid shared memory identifier.
shmaddr specifies an illegal address.
[EMFILE] The number of shared memory segments has reached
the system-wide limit.
[ENOMEM] There is not enough available data space for the
calling process to map the shared memory segment.
>
>> + pkt->data = pkt->buf->data;
>> + pkt->size = g->frame_size;
>> +
>> + return 0;
>> +}
>> +#endif
>
> Please comment the #endif.
Ok
>> +static int xcbgrab_draw_mouse(AVFormatContext *s, AVPacket *pkt,
>> + xcb_query_pointer_reply_t *p,
>> + xcb_get_geometry_reply_t *geo)
>> +{
>> + XCBGrabContext *gr = s->priv_data;
>> + uint32_t *cursor = NULL;
>> + uint8_t *image = pkt->data;
>> + int stride = gr->bpp / 8;
>> + xcb_xfixes_get_cursor_image_cookie_t cc;
>> + xcb_xfixes_get_cursor_image_reply_t *ci;
>> + int cx, cy, x, y, w, h, c_off, i_off;
>> +
>> + cc = xcb_xfixes_get_cursor_image(gr->conn);
>> + ci = xcb_xfixes_get_cursor_image_reply(gr->conn, cc, NULL);
>> + cursor = xcb_xfixes_get_cursor_image_cursor_image(ci);
>
> First you set cursor to NULL, then you set it again, redundantly.
Can be dropped.
>> + cursor += ci->width - w - c_off;
>> + image += (gr->width - w - i_off) * stride;
>
> nit: align more
>
>> +static int xcbgrab_read_close(AVFormatContext *s)
>> +{
>> + XCBGrabContext *g = s->priv_data;
>
> c or ctx sounds like a better variable name.
I can change all of this to c.
>> +static int pixfmt_from_pixmap_format(AVFormatContext *s, int depth)
>> +{
>> + while (length--) {
>> + if (fmt->depth == depth) {
>> + g->bpp = fmt->bits_per_pixel;
>> + switch (depth) {
>> + case 32:
>> + if (fmt->bits_per_pixel == 32) {
>> + g->frame_size *= 4;
>> + return AV_PIX_FMT_ARGB;
>> + }
>> + break;
>> + case 24:
>> + if (fmt->bits_per_pixel == 32) {
>> + g->frame_size *= 4;
>> + return AV_PIX_FMT_RGB32;
>> + } else if (fmt->bits_per_pixel == 24) {
>> + g->frame_size *= 3;
>> + return AV_PIX_FMT_RGB24;
>> + }
>> + break;
>> + case 16:
>> + if (fmt->bits_per_pixel == 16) {
>> + g->frame_size *= 2;
>> + return AV_PIX_FMT_RGB565;
>> + }
>> + break;
>> + case 15:
>> + if (fmt->bits_per_pixel == 16) {
>> + g->frame_size *= 2;
>> + return AV_PIX_FMT_RGB555;
>> + }
>> + break;
>> + case 8:
>> + if (fmt->bits_per_pixel == 8)
>> + return AV_PIX_FMT_RGB8;
>> + }
>> + }
>> + fmt++;
>> + }
>> + av_log(s, AV_LOG_ERROR, "Pixmap format not mappable\n");
>> +
>> + return AVERROR_PATCHWELCOME;
>> +}
>
> I think it would be cleaner if you returned enum AVPixelFormat.
it would be cleaner updating the pixel format from the function, I'll
update that way.
>> +av_cold static int xcbgrab_read_header(AVFormatContext *s)
>
> static av_cold
Right.
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
