[FFmpeg-devel] [PATCH] Bluray Subtitle Support, v2

[Diego Biurrun]

On Fri, Jul 24, 2009 at 05:57:24PM +1000, stev391 at exemail.com.au wrote:
> 
> Updated patch including Diego's, Aurelien's & Baptiste's comments.
> Thanks for the excellent and quick feedback.

You are most welcome.

> --- libavcodec/pgssubdec.c	(revision 0)
> +++ libavcodec/pgssubdec.c	(revision 0)
> @@ -0,0 +1,476 @@
> +/*
> + * PGS subtitle decoding

s/decoding/decoder/

> +typedef struct PGSSubPresentation {
> +    int x;
> +    int y;
> +    int video_w;
> +    int video_h;
> +    int id_number;
> +
> +} PGSSubPresentation;

The empty line at the end of your struct declarations looks weird and is
pointless.

> +            /* New Line. Check if correct pixels decoded,
> +               if not display warning and adjust bitmap
> +               pointer to correct new line position. */

> +        /* Skip 7 bytes (+1 for previous byte) of unknown:
> +               palette_update_flag (0x80),
> +               palette_id_to_use,
> +               Object Number (if > 0 determines if more data to process),
> +               object_id_ref (2 bytes),
> +               window_id_ref,
> +               composition_flag (0x80 - object cropped, 0x40 - object forced) */

I think stars to the left of the comments would make this more readable.
The second comment looks very much like code and keeps confusing me at
first glance.

> +    for(y = 0; y < h; y++) {
> +        for(x = 0; x < w; x++) {

for (

> +static int pgssub_display_end_segment(AVCodecContext *avctx,
> +                                        void *data,
> +                                        const uint8_t *buf, int buf_size)

indentation

> --- doc/general.texi	(revision 19505)
> +++ doc/general.texi	(working copy)
> @@ -633,6 +633,7 @@
>  @item DVB          @tab X @tab X @tab X @tab X
>  @item DVD          @tab X @tab X @tab X @tab X
>  @item XSUB         @tab   @tab   @tab X @tab X
> + at item PGS          @tab   @tab   @tab   @tab X
>  @end multitable

This is not in alphabetical order.

Diego