Re: [tyndur-devel] [PATCH v2] + libvideo: Header

[Kevin Wolf <kevin@xxxxxxxxxx>]

On Sat, Oct 10, 2009 at 03:31:45PM +0200, Alexander Siol wrote:
> Signed-off-by: Alexander Siol <alex@xxxxxxxxxx>
> ---
>  src/modules/include/video/commands.h |   80 ++++++++++
>  src/modules/include/video/video.h    |  269 ++++++++++++++++++++++++++++++++++
>  2 files changed, 349 insertions(+), 0 deletions(-)
>  create mode 100644 src/modules/include/video/commands.h
>  create mode 100644 src/modules/include/video/video.h
> 
> diff --git a/src/modules/include/video/commands.h b/src/modules/include/video/commands.h
> new file mode 100644
> index 0000000..2ec8c6e
> --- /dev/null
> +++ b/src/modules/include/video/commands.h
> @@ -0,0 +1,80 @@
> +/*  
> + * Copyright (c) 2009 The tyndur Project. All rights reserved.
> + *
> + * This code is derived from software contributed to the tyndur Project
> + * by Alexander Siol.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + * 1. Redistributions of source code must retain the above copyright
> + *    notice, this list of conditions and the following disclaimer.
> + * 2. Redistributions in binary form must reproduce the above copyright
> + *    notice, this list of conditions and the following disclaimer in the
> + *    documentation and/or other materials provided with the distribution.
> + *
> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS 
> + * ``AS IS'' AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED
> + * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
> + * PURPOSE ARE DISCLAIMED.  IN NO EVENT SHALL THE COPYRIGHT HOLDERS OR 
> + * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, 
> + * EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, 
> + * PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; 
> + * OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, 
> + * WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR 
> + * OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF 
> + * ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.*/

Viele Leerzeichen am Zeilenende.

> +
> +/// Kommandos
> +
> +enum video_commands {
> +    /// Kontextsteuerung

Das sollte kein Doxygen-Kommentar sein, sonst gehört der zur
CREATE_CONTEXT. Ich würde einen schönen C89-Kommentar vorschlagen.

> +
> +    /// Erzeugt einen neuen Treiberkontext
> +    VIDEO_CMD_CREATE_CONTEXT,
> +    /// Setzt den aktiven Treiberkontext
> +    VIDEO_CMD_USE_CONTEXT,
> +    /// Setzt das Geraet (per Nummer) im aktiven Treiberkontext
> +    VIDEO_CMD_USE_DEVICE,
> +    /// Setzt das aktive Ziel (Bitmap) im aktiven Treiberkontext
> +    VIDEO_CMD_USE_TARGET,
> +    /// Setzt die aktive Zeichenfarbe im aktiven Treiberkontext
> +    VIDEO_CMD_USE_COLOR,
> +    /// Setzt die aktive Raster-OP im aktiven Treiberkontext
> +    VIDEO_CMD_USE_ROP,
> +    /// Fordert einen Kommandobuffer fuer den Treiberkontext an.
> +    VIDEO_CMD_GET_CMD_BUFFER,
> +    /// Fordert die Ausfuehrung des Buffers an.
> +    VIDEO_CMD_DO_CMD_BUFFER,
> +
> +    /// Ger??teabfrage

Umlaut.

> +
> +    /// Fragt die Anzahl Geraete ab
> +    VIDEO_CMD_GET_NUM_DEVICES,
> +    /// Fragt die Anzahl der Anzeigen am Geraet ab
> +    VIDEO_CMD_GET_NUM_DISPLAYS,
> +    /// Fragt die Displaymodi ab
> +    VIDEO_CMD_GET_DISPLAY_MODES,
> +
> +    /// Anderes
> +
> +    /// Setzt den Modus
> +    /// FIXME Umbenennen
> +    VIDEO_CMD_SET_RESOLUTION,

Mit diesem Trivial-FIXME drin kriegst du aber kein Ack. ;-)

> +
> +    /// Bitmaps
> +    /// Erzeugt ein Bitmap
> +    VIDEO_CMD_CREATE_BITMAP,
> +    /// Zerstoert ein Bitmap
> +    VIDEO_CMD_DESTROY_BITMAP,
> +    /// Erzeugt eine Bitmap auf den Display-Frontbuffer
> +    VIDEO_CMD_CREATE_FRONTBUFFER_BITMAP,
> +
> +    /// Primitiven Zeichnen
> +    VIDEO_CMD_DRAW_PIXEL,
> +    VIDEO_CMD_DRAW_RECTANGLE,
> +    VIDEO_CMD_DRAW_ELLIPSE,
> +    VIDEO_CMD_DRAW_LINE,
> +    VIDEO_CMD_DRAW_BITMAP,
> +    VIDEO_CMD_DRAW_BITMAP_PART,
> +};
> \ No newline at end of file

Irgendein bestimmter Grund, warum das nicht in der video.h sein muss?
Ein eigener Header fuer genau einen enum kommt mir irgendwie übertrieben
vor.

> diff --git a/src/modules/include/video/video.h b/src/modules/include/video/video.h
> new file mode 100644
> index 0000000..b2881d2
> --- /dev/null
> +++ b/src/modules/include/video/video.h
> @@ -0,0 +1,269 @@
> +/*  
> + * Copyright (c) 2009 The tyndur Project. All rights reserved.
> + *
> + * This code is derived from software contributed to the tyndur Project
> + * by Alexander Siol.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + * 1. Redistributions of source code must retain the above copyright
> + *    notice, this list of conditions and the following disclaimer.
> + * 2. Redistributions in binary form must reproduce the above copyright
> + *    notice, this list of conditions and the following disclaimer in the
> + *    documentation and/or other materials provided with the distribution.
> + *
> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> + * ``AS IS'' AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED
> + * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
> + * PURPOSE ARE DISCLAIMED.  IN NO EVENT SHALL THE COPYRIGHT HOLDERS OR
> + * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL,
> + * EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO,
> + * PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS;
> + * OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY,
> + * WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR
> + * OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF
> + * ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.*/
> +
> +#ifndef __LIBVIDEO_VIDEO_H__
> +#define __LIBVIDEO_VIDEO_H__
> +
> +#include <types.h>
> +#include <rpc.h>
> +
> +#define VIDEORPC_GET_DWORD(size, data) \
> +    rpc_get_dword(context->driverpid, "VIDEODRV", size, data)
> +
> +#define REQUIRE_CMD_BUFFER(length) \
> +    do { \
> +    if (context->cmdbuffer == 0) return -1; \
> +    if (context->cmdbufferlen < context->cmdbufferpos + length) \
> +        libvideo_do_command_buffer(); \
> +    } while (0);

Codestil (Einrückung, if ohne Klammern)

Gibt es einen Grund, warum es ein Makro sein muss und nicht eine
static-inline-Funktion?

> +
> +#define REQUIRE_CMD_BUFFER_RETURN_VOID(length) \
> +    do { \
> +    if (context->cmdbuffer == 0) return; \
> +    if (context->cmdbufferlen < context->cmdbufferpos + length) \
> +        libvideo_do_command_buffer(); \
> +    } while (0);

Auch ein Kandidat für static inline.

> +
> +#define SET_CMD_BUFFER(value) \
> +    context->cmdbuffer[context->cmdbufferpos++] = value

do { ... } while(0)

> +
> +
> +typedef struct {
> +    pid_t driverpid;
> +    uint32_t drivercontext;
> +    uint32_t* cmdbuffer;
> +    uint32_t cmdbufferpos;
> +    uint32_t cmdbufferlen;
> +    uint32_t cmdbufferid;
> +} driver_context_t;

Hier wäre ein Kommentar nett, was man sich unter so einem Kontext
überhaupt vorzustellen hat.

> +
> +typedef struct {
> +    uint32_t id;
> +    uint32_t width;
> +    uint32_t height;
> +} video_bitmap_t;
> +
> +
> +// FIXME: Muss immer mit CDI (cdi/video.h) abgeglichen werden!
> +
> +typedef enum {
> +    VIDEO_ROP_COPY,
> +    VIDEO_ROP_OR,
> +    VIDEO_ROP_AND,
> +    VIDEO_ROP_XOR,
> +    // TODO: Erweitern
> +} rop_t;
> +
> +/// Kontextsteuerung
> +
> +/**
> + * Erstellt einen neuen Kontext fuer den angebenen Treiber
> + * @param driver String, Name des Treibers (= Prozessname)

Servicename? Prozesse haben maximal IDs oder Kommandozeilen.

> + * @return Zeiger auf Kontext. NULL im Fehlerfall.
> + */
> +driver_context_t* libvideo_create_driver_context(char* driver);
> +
> +/**
> + * Setzt den aktuell aktiven Kontext
> + * @param context Zeiger auf den zu aktivierenden Kontext.
> + */
> +void libvideo_use_driver_context(driver_context_t* context);
> +
> +/// Kontextabhaengig
> +
> +/**
> + * Setzt das aktive Geraet im aktuellen Kontext
> + * @param devicenum Geraetenummer.
> + * @return 0 bei Erfolg, sonst -1
> + */
> +int libvideo_change_device(int devicenum);
> +
> +/**
> + * Setzt das aktive Ziel(bitmap) im aktuellen Kontext
> + * @param bitmap Zeiger auf Bitmap.
> + * @return 0 bei Erfolg, sonst -1
> + */
> +int libvideo_change_target(video_bitmap_t *bitmap);

Stern gehört zum Typ.

> +
> +/**
> + * Setzt die Zeichenfarbe im aktuellen Kontext
> + * @param alpha Alpha-Anteil (0-255)
> + * @param red Rot-Anteil (0-255)
> + * @param green Gruen-Anteil (0-255)
> + * @param blue Blau-Anteil (0-255)
> + * @return 0 bei Erfolg, sonst -1
> + */
> +int libvideo_change_color(char alpha, char red, char green, char blue);

uint8_t statt char (oder wenigstens unsigned char, sonst ist nämlich der
Kommentar Blödsinn). Wäre Alpha als letzter Parameter nicht üblicher?

> +
> +/**
> + * Setzt die Raster-OP fuer weitere Zeichenvorgaenge im aktuellen Kontext
> + * @param rop Die Raster-OP
> + * @return 0 bei Erfolg, sonst -1
> + */
> +int libvideo_change_rop(rop_t rop);
> +
> +/**
> + * Fordert einen neuen Kommandobuffer fuer den Kontext an.
> + * @param length Die Laenge des Zeichenbuffers in 4 Bytes.
> + * @return 0 bei Erfolg, sonst -1
> + */
> +int libvideo_get_command_buffer(size_t length);
> +
> +/**
> + * Gibt dem Treiber die Anweisung den Buffer auszufuehren.
> + * Achtung, dies wird auch automatisch getan wenn der Buffer voll ist!
> + * Achtung, nicht nach jedem Zeichenvorgang aufrufen - kostet Geschwindigkeit!
> + * @return 0 bei Erfolg, sonst -1
> + */
> +int libvideo_do_command_buffer(void);
> +
> +/// Geraeteabfrage
> +
> +/**
> + * Gibt die Anzahl der Geraete am Treiber des aktuellen Kontexts zurueck.
> + * @return die Anzahl der Geraete des Treibers.
> + */
> +int libvideo_get_num_devices(void);
> +
> +/**
> + * Gibt die Zahl der Anzeigen am angegebenen Geraete im aktuellen Kontext zurueck
> + * @param device Geraetenummer
> + * @return Anzahl der Anzeigen am Grafikadapter
> + */
> +int libvideo_get_num_displays(int device);

Wäre eine struct video_device nicht schlauer?

> +
> +/**
> + * Liefert ein Array mit Modusinformationen
> + * @param device Geraetenummer
> + * @param display Anzeigennummer
> + * @return dword[0] enthaelt die Anzahl an Modi. Fuer jeden Modus:
> + *  dword[i + 1] Breite (px), dword[i + 2] Hoehe (px), dword[i + 3] Bits per Pixel,
> + *  dword[i + 4] Bildwiederholrate (VSync Hz), 0 fuer automatische Einstellung.
> + */
> +dword* libvideo_get_modes(int device, int display);
> +
> +/// Bitmaps
> +/**
> + * Erzeugt ein Bitmap, laedt ggf. Initialisierungsdaten zum Treiber.
> + * Ohne Initialisierung erhaelt man ein schwarzes Bitmap.
> + * @param width Breite in Pixel
> + * @param height Hoehe in Pixel
> + * @param length Laenge der Nutzdaten, 0 fuer keine.
> + * @param data Zeiger auf Datenbuffer, Pixelformat wird als 32-bit ARGB angenommen
> + */
> +video_bitmap_t* libvideo_create_bitmap(int width, int height, 

Leerzeichen am Zeilenende

> +    size_t data_length, void* data);
> +
> +/**
> + * Gibt ein Bitmap wieder frei.
> + * @param bitmap Das Bitmap.
> + */
> +void libvideo_destroy_bitmap(video_bitmap_t *bitmap);

Stern

> +
> +/**
> + * Holt ein Bitmap-Objekt fuer den Anzeigenfrontbuffer
> + * @param display Die Anzeige deren Frontbuffer einen interessiert
> + * @return Zeiger auf Bitmap, NULL im Fehlerfall
> + * @note Es kann global nur eine Bitmap fuer Frontbufferzugriff geben!
> + */
> +video_bitmap_t* libvideo_get_frontbuffer_bitmap(int display);
> +
> +/// Primitiven Zeichnen

Doxygen-Kommentar, der keiner sein sollte

> +
> +/**
> + * Zeichnet einen Pixel in der gesetzten Farbe auf das aktuelle Ziel.
> + * @param x X-Koordinate
> + * @param y Y-Koordinate
> + */
> +void libvideo_draw_pixel(int x, int y);
> +
> +/**
> + * Zeichnet einen Rechteck in der gesetzten Farbe auf das aktuelle Ziel.
> + * @param x X-Koordinate
> + * @param y Y-Koordinate
> + * @param width Breite in Pixel
> + * @param height Hoehe in Pixel
> + */
> +void libvideo_draw_rectangle(int x, int y, int width, int height);
> +
> +/**
> + * Zeichnet eine Ellipse (die das spezifizierte Rechteck bestmoeglich ausfuellt)
> + * in der gesetzten Farbe auf das aktuelle Ziel.
> + * @param x X-Koordinate
> + * @param y Y-Koordinate
> + * @param width Breite in Pixel
> + * @param height Hoehe in Pixel
> + */
> +void libvideo_draw_ellipse(int x, int y, int width, int height);
> +
> +
> +/**
> + * Zeichnet eine Linie in der gesetzten Farbe auf das aktuelle Ziel.
> + * @param x1 Start-X-Koordinate
> + * @param y1 Start-Y-Koordinate
> + * @param x2 End-X-Koordinate
> + * @param y2 End-Y-Koordinate
> + */
> +void libvideo_draw_line(int x1, int y1, int x2, int y2);
> +
> +/**
> + * Zeichnet eine Bitmap auf das aktuelle Ziel.
> + * @param bitmap die Quell-Bitmap
> + * @param x X-Koordinate
> + * @param y Y-Koordinate
> + */
> +void libvideo_draw_bitmap(video_bitmap_t *bitmap, int x, int y);

Stern

> +
> +/**
> + * Zeichnet einen Bitmapausschnitt auf das aktuelle Ziel.
> + * @param bitmap die Quell-Bitmap
> + * @param x Ziel-X-Koordinate
> + * @param y Ziel-Y-Koordinate
> + * @param srcx Start-X-Koordinate des Quellrechtecks
> + * @param srcy Start-Y-Koordinate des Quellrechtecks
> + * @param width Breite des zu kopierenden Rechtecks in Pixel
> + * @param height Hoehe des zu kopierenden Rechtecks in Pixel
> + */
> +void libvideo_draw_bitmap_part(video_bitmap_t *bitmap, int x, int y, int srcx,
> +    int srcy, int width, int height);

Stern

> +
> +/// Anderes
> +
> +/**
> + * Setzt die Aufloesung einer Anzeige
> + * @param display Die Anzeige
> + * @param width Breite
> + * @param height Hoehe
> + * @param depth Gewuenschte Farbtiefe (in BPP)
> + * @return 0 bei Erfolg, sonst -1
> + * @note Die Farbtiefe ist nur ein Richtwert - es wird gegebenfalls eine andere
> + *  verwendet, allerdings werden die Farbwerte transparent umgerechnet.
> + */
> +int libvideo_change_display_resolution(int display, int width, int height,
> +    int depth);

Sollte wohl auch umbenannt werden, wenn das FIXME oben noch gilt.