[vlc-devel] [PATCH] mp4: Handle colr atom

[Francois Cartegnie]

Le 16/04/2016 17:07, Hugo Beauzée-Luyssen a écrit :

> +    const MP4_Box_t *p_colr = MP4_BoxGet( p_sample, "colr" );
> +    if ( p_colr != NULL )
> +    {
> +        if ( BOXDATA(p_colr)->i_type == ATOM_nclc || BOXDATA(p_colr)->i_type == ATOM_nclx )
> +        {
> +            bool b_fullrange = BOXDATA(p_colr)->i_type == ATOM_nclx &&
> +                    (BOXDATA(p_colr)->nclx.i_full_range >> 7) != 0;
> +
> +            switch ( BOXDATA( p_colr )->nclc.i_primary_idx )
> +            {
> +            case 1: p_track->fmt.video.primaries = COLOR_PRIMARIES_BT709; break;
> +            case 5: p_track->fmt.video.primaries = COLOR_PRIMARIES_BT601_625; break;
> +            case 6: p_track->fmt.video.primaries = COLOR_PRIMARIES_BT601_525; break;
> +            default: p_track->fmt.video.primaries = COLOR_PRIMARIES_UNDEF; break;

default value should have been set by es_format_Init

> +static int MP4_ReadBox_colr( stream_t *p_stream, MP4_Box_t *p_box )
> +{
> +    MP4_READBOX_ENTER( MP4_Box_data_colr_t, NULL );
> +    MP4_GETFOURCC( p_box->data.p_colr->i_type );
> +    if ( p_box->data.p_colr->i_type == ATOM_nclc || p_box->data.p_colr->i_type == ATOM_nclx )
> +    {
> +        MP4_GET2BYTES( p_box->data.p_colr->nclc.i_primary_idx );
> +        MP4_GET2BYTES( p_box->data.p_colr->nclc.i_transfer_function_idx );
> +        MP4_GET2BYTES( p_box->data.p_colr->nclc.i_matrix_idx );
> +        if ( p_box->data.p_colr->i_type == ATOM_nclx )
> +            MP4_GET1BYTE( p_box->data.p_colr->nclx.i_full_range );
> +    }
> +    else
> +    {
> +#ifdef MP4_VERBOSE
> +        msg_Warn( p_stream, "Unhandled colr type: %4.4s", (char*)&p_box->data.p_colr->i_type );
> +#endif

Well, your box is still valid. Your app just need to reject its value,
which already does.
Rejecting it here will get it unlisted, then harder to debug.

> +        MP4_READBOX_EXIT( 0 );
> +    }
> +    MP4_READBOX_EXIT( 1 );


No please no global boxes. We need valid parenting.

> +typedef struct MP4_Box_data_colr_s
> +{
> +    uint32_t i_type;
> +    union
> +    {
> +        MP4_Box_Data_nclc_t nclc;
> +        MP4_Box_Data_nclx_t nclx;
> +    };
> +} MP4_Box_data_colr_t;

Unsure of the need for union here. But if you want to make colr a whole
atom, an not use it as a parent container, then you need to rename/move
out ATOM_ncl* from atoms list.

Francois