[FFmpeg-devel] [PATCH] Support for Dirac in ogg

[Diego Biurrun]

On Wed, Nov 05, 2008 at 03:32:22AM -0500, David Conrad wrote:
>
> Attached adds demuxing support for dirac in ogg files. It uses a couple 
> functions for the soc dirac decoder to parse the header, updated to the 
> latest specification.

This begs for the question: What about the rest of the Dirac decoder?

The build system part of the patch is OK, some nits below.

> --- /dev/null
> +++ b/libavcodec/dirac.c
> @@ -0,0 +1,268 @@
> +
> +/**
> + * @file dirac.c
> + * Dirac Decoder

decoder

> +/* Defaults for source parameters.  */

lowercase, drop period

> + * Parse the source parameters in the sequence header

.

> +    /* Framerate.  */

framerate

> +    /* Override clean area.  */

Are these double spaces at the end of comments on purpose?  Not that it
matters..

> +    /* Color spec.  */

color spec

> +/**
> + * Parse the sequence header

.

> --- /dev/null
> +++ b/libavcodec/dirac.h
> @@ -0,0 +1,109 @@
> +
> +/**
> + * @file dirac.h
> + * Interfaces to Dirac Decoder/Encoder

interfaces to Dirac decoder/encoder

> +typedef enum {
> +    COLOR_PRIMARY_HDTV,         ///< ITU-R BT. 709, also computer/web/sRGB
> +    COLOR_PRIMARY_SDTV_525,     ///< SMPTE 170M, 525 primaries
> +    COLOR_PRIMARY_SDTV_625,     ///< EBU Tech 3213-E, 625 primaries
> +    COLOR_PRIMARY_DCINEMA,      ///< SMPTE 428.1, CIE XYZ
> +} color_primary_t;
> +
> +typedef enum {
> +    COLOR_MATRIX_HDTV,          ///< ITU-R BT.709, also computer/web
> +    COLOR_MATRIX_SDTV,          ///< ITU-R BT.601
> +    COLOR_MATRIX_REVERSIBLE,    ///< ITU-T H.264
> +} color_matrix_t;
> +
> +typedef enum {
> +    TRANSFER_FUNC_TV,
> +    TRANSFER_FUNC_EXTENDED_GAMUT,
> +    TRANSFER_FUNC_LINEAR,
> +    TRANSFER_FUNC_DCI_GAMMA
> +} transfer_func_t;

The _t namespace is reserved by POSIX IIRC, use something else.

> +    /* Information about the frames.  */

lowercase, drop period

> +    unsigned int luma_width;           ///< the luma component width
> +    unsigned int luma_height;          ///< the luma component height
> +    /** Choma format: 0: 4:4:4, 1: 4:2:2, 2: 4:2:0 */

chroma

> +    /* Interlacing.  */
> +    /* Clean area.  */

see above

> +    uint16_t clean_width;
> +    uint16_t clean_height;
> +    uint16_t clean_left_offset;
> +    uint16_t clean_right_offset;

You should '#include <stdint.h>' for these.

> +    /* Calculated:  */

Why the colon?

> +    /* Luma and chroma offsets.  */

see above

Diego