Re: [tyndur-devel] [PATCH 2/6] + libvideo: Header

[Kevin Wolf <kevin@xxxxxxxxxx>]

On Mon, Sep 21, 2009 at 12:54:09PM +0200, Alexander Siol wrote:
> Signed-off-by: Alexander Siol <alex@xxxxxxxxxx>
> ---
>  src/modules/include/video/commands.h |   63 ++++++++++++
>  src/modules/include/video/drawing.h  |  187 ++++++++++++++++++++++++++++++++++
>  src/modules/include/video/rop.h      |   41 ++++++++
>  src/modules/include/video/video.h    |  105 +++++++++++++++++++
>  4 files changed, 396 insertions(+), 0 deletions(-)
>  create mode 100644 src/modules/include/video/commands.h
>  create mode 100644 src/modules/include/video/drawing.h
>  create mode 100644 src/modules/include/video/rop.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..70516f8
> --- /dev/null
> +++ b/src/modules/include/video/commands.h
> @@ -0,0 +1,63 @@
> +/*  
> + * 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.*/

Trailing Whitespace im Lizenzheader. Fehlende Includeguards drunter.

> +
> +/// Kommandos
> +
> +enum video_commands {
> +VIDEO_CMD_NULL = 0,

Einrückung?

Soll VIDEO_CMD_NULL ein NOP sein? Vielleicht wäre es sinnvoll, die
einzelnen Befehle hier auch zu kommentieren, was sie genau machen. Oder
kommt das noch an anderer Stelle?

> +
> +/// Kontextsteuerung
> +VIDEO_CMD_CREATE_CONTEXT,
> +VIDEO_CMD_USE_CONTEXT,
> +VIDEO_CMD_USE_DEVICE,
> +VIDEO_CMD_USE_TARGET,

Was ist denn ein Target? Das, was bei GET_NUM_DISPLAYS Display heißt?
Wenn ja, konsistenter benennen; wenn nein, kommentieren, was es sein
soll. ;-)

> +VIDEO_CMD_USE_COLOR,
> +VIDEO_CMD_USE_ROP,
> +VIDEO_CMD_GET_CMD_BUFFER,
> +VIDEO_CMD_DO_CMD_BUFFER,
> +
> +/// Ger??teabfrage

Kaputter Umlaut

> +VIDEO_CMD_GET_NUM_DEVICES,
> +VIDEO_CMD_GET_NUM_DISPLAYS,
> +VIDEO_CMD_GET_DISPLAY_MODES,
> +
> +/// Anderes
> +VIDEO_CMD_SET_RESOLUTION,

SET_MODE? Oder wird die Farbtiefe, Bildwiederholfrequenz usw. unabhängig
gesetzt?

> +
> +/// Bitmaps
> +VIDEO_CMD_CREATE_BITMAP,
> +VIDEO_CMD_DESTROY_BITMAP,
> +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

Was benutzt denn du für einen Editor, dass das abschließende \n fehlt?

Ist es wirklich so sinnvoll, nur für dieses eine enum eine eigene
Headerdatei zu nehmen?

> diff --git a/src/modules/include/video/drawing.h b/src/modules/include/video/drawing.h
> new file mode 100644
> index 0000000..440acf9
> --- /dev/null
> +++ b/src/modules/include/video/drawing.h
> @@ -0,0 +1,187 @@
> +/*  
> + * Copyright (c) 2008 The LOST Project. All rights reserved.
> + *
> + * This code is derived from software contributed to the LOST Project
> + * by Mathias Gottschlag.
> + *
> + * 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.
> + * 3. All advertising materials mentioning features or use of this software
> + *    must display the following acknowledgement:
> + *     This product includes software developed by the LOST Project
> + *     and its contributors.
> + * 4. Neither the name of the LOST Project nor the names of its
> + *    contributors may be used to endorse or promote products derived
> + *    from this software without specific prior written permission.
> + *
> + * 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.
> + */

Hier nur die Leerzeichen

> +
> +#ifndef DRAWING_H
> +#define DRAWING_H
> +
> +/**
> + * Startet einen Videokommandostream. Kommandos werden gebuffert per Shared
> + * Memory uebertragen.
> + */
> +void video_start_command(void);
> +/**
> + * Beendet einen Videokommandostream und sendet ihn an den Treiber.
> + */
> +void video_end_command(void);
> +
> +/**
> + * Raster Ops kontrollieren, wie etwas gezeichnet wird
> + */
> +typedef enum video_rop {
> +    /// Normales Schreiben aufs Ziel, Untergrund wird ueberschrieben
> +    ROP_COPY,
> +    /// Zwischen Untergrund und zu schreibendem wird ein "or" durchgef??hrt
> +    /// Beispiel: Malen von Teilen mittels einer weissen Maske
> +    ROP_OR,
> +    /// Zwischen Untergrund und zu schreibendem wird ein "and" durchgef??hrt
> +    /// Beispiel: Loeschen von Teilen mittels einer schwarzen Maske
> +    ROP_AND,
> +    /// Zwischen Untergrund und zu schreibendem wird ein "xor" durchgef??hrt

Umlaute kaputt

> +    /// Beispiel: Invertieren eines Bildes durch Uebermalen mit weissem Rechteck
> +    ROP_XOR
> +} video_rop_t;
> +
> +/**
> + * Stellt den Raster Op ein, der zum Zeichnen benutzt wird
> + * Muss zwischen video_start_command und video_end_command aufgerufen werden.
> + */
> +void video_set_rop(video_rop_t rop);
> +
> +/**
> + * Stellt das Ziel ein, auf das geschrieben wird.
> + * Muss zwischen video_start_command und video_end_command aufgerufen werden.
> + * @param bitmap Zielbitmap. Wenn gleich 0, dann wird auf den Bildschirm
> + * geschrieben.
> + */
> +void video_set_target(int bitmap); // 0 = Screen
> +
> +/**
> + * Zeichnet einen farbigen Punkt
> + * Muss zwischen video_start_command und video_end_command aufgerufen werden.
> + * @param color Farbe des Punktes
> + * @param x X-Koordinate
> + * @param y Y-Koordinate
> + */
> +void video_draw_dot(int color, int x, int y);

draw_pixel? "dot" hat erstmal keine definierte Größe, imho.

> +/**
> + * Zeichnet ein farbiges Rechteck
> + * Muss zwischen video_start_command und video_end_command aufgerufen werden.
> + * @param color Farbe des Rechtecks
> + * @param x X-Koordinate
> + * @param y Y-Koordinate
> + * @param width Breite des Rechtecks
> + * @param height Hoehe des Rechtecks
> + */
> +void video_draw_solid_rect(int color, int x, int y, int width, int height);
> +/**
> + * Zeichnet eine farbige Linie
> + * Muss zwischen video_start_command und video_end_command aufgerufen werden.
> + * @param color Farbe der Linie
> + * @param x1 X-Koordinate des Startpunkts
> + * @param y1 Y-Koordinate des Startpunkts
> + * @param x2 X-Koordinate des Endpunkts
> + * @param y2 Y-Koordinate des Endpunkts
> + */
> +void video_draw_line(int color, int x1, int y1, int x2, int y2);
> +/**
> + * Zeichnet eine farbige Ellipse
> + * Muss zwischen video_start_command und video_end_command aufgerufen werden.
> + * @param color Farbe der Ellipse
> + * @param x X-Koordinate
> + * @param y Y-Koordinate
> + * @param width Breite der Ellipse
> + * @param height Hoehe der Ellipse
> + */
> +void video_draw_ellipse(int color, int x, int y, int width, int height);
> +
> +/**
> + * Zeichnet ein komplettes Bitmap aufs Ziel. Das Bitmap muss vorher via
> + * video_upload_bitmap geladen worden sein.
> + * Muss zwischen video_start_command und video_end_command aufgerufen werden.
> + * @param bitmap Zu benutzendes Bitmap
> + * @param x X-Koordinate
> + * @param y Y-Koordinate
> + */
> +void video_draw_bitmap(int bitmap, int x, int y);
> +/**
> + * Zeichnet einen Teil eines Bitmaps aufs Ziel. Das Bitmap muss vorher via
> + * video_upload_bitmap geladen worden sein.
> + * Muss zwischen video_start_command und video_end_command aufgerufen werden.
> + * @param bitmap Zu benutzendes Bitmap
> + * @param x X-Koordinate
> + * @param y Y-Koordinate
> + * @param srcx X-Koordinate des zu zeichnenden Teils
> + * @param srcy Y-Koordinate des zu zeichnenden Teils
> + * @param srcwidth Breite des zu zeichnenden Teils
> + * @param srcheight Hoehe des zu zeichnenden Teils
> + */
> +void video_draw_bitmap_part(int bitmap, int x, int y, int srcx, int srcy, int srcwidth, int srcheight);
> +/**
> + * Zeichnet einen Teil aus dem Bildschirm aufs Ziel.

Bildschirm, Ziel? Ich glaube, du musst die Datei erstmal mit einem
großen Kommentar zur Terminologie anfangen. ;-) Also für mich ist der
Bildschirm die endgültige Ausgabe, von dem will ich doch nichts
wegkopieren?

> + * Muss zwischen video_start_command und video_end_command aufgerufen werden.
> + * @param x X-Koordinate
> + * @param y Y-Koordinate
> + * @param srcx X-Koordinate des zu zeichnenden Teils
> + * @param srcy Y-Koordinate des zu zeichnenden Teils
> + * @param srcwidth Breite des zu zeichnenden Teils
> + * @param srcheight Hoehe des zu zeichnenden Teils
> + */
> +void video_copy_screen(int x, int y, int srcx, int srcy, int srcwidth, int srcheight);
> +
> +// Bitmapformate
> +#define BITMAP_FORMAT_MONO_1 0
> +#define BITMAP_FORMAT_MONO_8 1
> +#define BITMAP_FORMAT_R8G8B8_24 2
> +#define BITMAP_FORMAT_B8G8R8_24 3
> +#define BITMAP_FORMAT_R8G8B8_32 4
> +#define BITMAP_FORMAT_B8G8R8_32 5
> +#define BITMAP_FORMAT_R5G6B5_16 6
> +#define BITMAP_FORMAT_B5G6R5_16 7
> +#define BITMAP_FORMAT_R5G5B5_16 8
> +#define BITMAP_FORMAT_B5G5R5_16 9
> +#define BITMAP_FORMAT_GREY_8 10

enum?

> +
> +/**
> + * Laedt ein Bitmap in den Videotreiber.
> + * @param data Pixeldaten oder 0, wenn das Bitmap nicht initialisiert werden soll
> + * @param width Breite des Bitmaps
> + * @param height Hoehe des Bitmaps
> + * @param format Format der Pixeldaten
> + * @return ID des Bitmaps

Schiefgehen kann das nicht?

> + */
> +int video_upload_bitmap(void *data, int width, int height, int format);
> +/**
> + * Loescht das Bitmap mit der angegebenen ID
> + * @param bitmap Zu loeschendes Bitmap
> + */
> +void video_delete_bitmap(int bitmap);
> +/**
> + * Loescht alle Bitmaps, die zuvor via video_upload_bitmap in den Videotreiber
> + * geladen wurden
> + */
> +void video_delete_all_bitmaps(void);
> +
> +#endif
> +
> diff --git a/src/modules/include/video/rop.h b/src/modules/include/video/rop.h
> new file mode 100644
> index 0000000..0e5724e
> --- /dev/null
> +++ b/src/modules/include/video/rop.h
> @@ -0,0 +1,41 @@
> +/*  
> + * 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 __VIDEO_ROP_H__
> +#define __VIDEO_ROP_H__
> +
> +// 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;

Oben hatten wir schonmal einen video_rop_t. Eine eigene Headerdatei
sieht auch übertrieben aus.

> +
> +#endif /* __VIDEO_ROP_H__ */
> \ No newline at end of file
> diff --git a/src/modules/include/video/video.h b/src/modules/include/video/video.h
> new file mode 100644
> index 0000000..7b303e9
> --- /dev/null
> +++ b/src/modules/include/video/video.h
> @@ -0,0 +1,105 @@
> +/*  
> + * 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>
> +#include <video/rop.h>
> +
> +#define VIDEORPC_GET_DWORD(size, data) \
> +    rpc_get_dword(context->driverpid, "VIDEODRV", size, data);

Ich würde static inline vorziehen.

Aber wenn schon Makros, solltest du drauf achten, dass sich das alles
verhält wie eine normale Funktion. Beim VIDEORPC_GET_DWORD ist
beispielsweile der ; zuviel, so dass man es nicht in einer if-Bedingung
einsetzen kann.

> +
> +#define REQUIRE_CMD_BUFFER(length) \
> +    if (context->cmdbuffer == 0) return -1; \
> +    if (context->cmdbufferlen < context->cmdbufferpos + length) \
> +        libvideo_do_command_buffer();
> +
> +#define REQUIRE_CMD_BUFFER_RETURN_VOID(length) \
> +    if (context->cmdbuffer == 0) return; \
> +    if (context->cmdbufferlen < context->cmdbufferpos + length) \
> +        libvideo_do_command_buffer();

Mit zwei Anweisungen im Makro wird das noch lustiger. Überleg dir mal,
was hier passiert:

if (foo == bar)
    REQUIRE_CMD_BUFFER(1337);

Wenn man sowas machen will, definiert man das Makro üblicherweise als
do { ... } while (0)

> +
> +#define SET_CMD_BUFFER(value) \
> +    context->cmdbuffer[context->cmdbufferpos++] = value;
> +
> +
> +typedef struct {
> +    int driverpid;
> +    int drivercontext;
> +    dword *cmdbuffer;

uint32_t

> +    int cmdbufferpos;
> +    int cmdbufferlen;
> +    dword cmdbufferid;

Nochmal

> +} driver_context_t;
> +
> +typedef struct {
> +    int id;
> +    int width;
> +    int height;
> +} video_bitmap_t;
> +
> +extern driver_context_t *context;

Eine globale Variable namens context? Also viel schlechter geht es nicht
mehr. Namenskonflikte sind so garantiert...

> +
> +driver_context_t* libvideo_create_driver_context(char* driver);
> +void libvideo_use_driver_context(driver_context_t* context);
> +
> +/// Kontextabhaengig
> +int libvideo_change_device(int devicenum);
> +int libvideo_change_target(video_bitmap_t *bitmap);
> +int libvideo_change_color(char alpha, char red, char green, char blue);
> +int libvideo_change_rop(rop_t rop);
> +int libvideo_get_command_buffer(size_t length);
> +int libvideo_do_command_buffer(void);
> +
> +/// Ger??teabfrage

Umlaut

> +int libvideo_get_num_devices(void);
> +int libvideo_get_num_displays(int device);
> +dword* libvideo_get_modes(int device, int display);
> +
> +/// Bitmaps
> +video_bitmap_t* libvideo_create_bitmap(int width, int height, 

Leerzeichen am Ende

> +    size_t data_length, void* data);
> +void libvideo_destroy_bitmap(video_bitmap_t *bitmap);
> +video_bitmap_t* libvideo_get_frontbuffer_bitmap(int display);
> +
> +/// Primitiven Zeichnen
> +void libvideo_draw_pixel(int x, int y);
> +void libvideo_draw_rectangle(int x, int y, int width, int height);
> +void libvideo_draw_ellipse(int x, int y, int width, int height);
> +void libvideo_draw_line(int x1, int y1, int x2, int y2);
> +void libvideo_draw_bitmap(video_bitmap_t *bitmap, int x, int y);
> +void libvideo_draw_bitmap_part(video_bitmap_t *bitmap, int x, int y, int srcx,
> +    int srcy, int width, int height);
> +
> +/// Anderes
> +int libvideo_change_display_resolution(int display, int width, int height,
> +    int depth);
> +
> +
> +#endif /* __LIBVIDEO_VIDEO_H__ */
> \ No newline at end of file
> -- 
> 1.6.0.4
> 

> _______________________________________________
> tyndur-devel mailing list
> tyndur-devel@xxxxxxxxxx
> http://list.tyndur.org/mailman/listinfo/tyndur-devel