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? Do configurations
exist where one part is available w/o the others?
> --- /dev/null
> +++ b/libavdevice/xcbgrab.c
> @@ -0,0 +1,646 @@
> +/*
> + * XCB input
> + * Copyright (c) 2014 Luca Barbato <[email protected]>
nit: (C)
> +#include "config.h"
> +
> +#include <stdlib.h>
> +#include <xcb/xcb.h>
> +#include <xcb/composite.h>
> +#include <xcb/xcb_event.h>
> +#include <xcb/xfixes.h>
> +#if HAVE_SYS_SHM_H
> +# include <sys/shm.h>
> +# include <xcb/shm.h>
> +#endif
Does it work w/o the shm headers?
> +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 {}
> + } else {
> + int left = x + f;
> + int right = x + w - f;
> + int top = y + f;
> + int bottom = y + h + f;
> + if (p_x > right) {
> + x += p_x - right;
> + } else if (p_x < left) {
> + x -= left - p_x;
> + }
> + if (p_y > bottom) {
> + y += p_y - bottom;
> + } else if (p_y < top) {
> + y -= top - p_y;
> + }
> + }
nit: extra {}
> + g->x = FFMIN(FFMAX(0, x), geo->width - w);
> + g->y = FFMIN(FFMAX(0, y), geo->height - h);
nit: align -
> +static int xcbgrab_frame(AVFormatContext *s, AVPacket *pkt)
> +{
> + XCBGrabContext *g = s->priv_data;
> + xcb_get_image_cookie_t iq;
> + xcb_get_image_reply_t *img;
> + xcb_drawable_t drawable = g->screen->root;
> + uint8_t *data;
> + int length;
> +
> + 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.
> + 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?
> + 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.
> + 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.
> + pkt->data = pkt->buf->data;
> + pkt->size = g->frame_size;
> +
> + return 0;
> +}
> +#endif
Please comment the #endif.
> +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.
> + 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.
> +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.
> +av_cold static int xcbgrab_read_header(AVFormatContext *s)
static av_cold
Diego
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
