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

[Hugo Beauzée-Luyssen]

On 04/16/2016 05:41 PM, Francois Cartegnie wrote:
> 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

Fair enough

>
>> +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.

True, fixed.

>
>> +        MP4_READBOX_EXIT( 0 );
>> +    }
>> +    MP4_READBOX_EXIT( 1 );
>
>
> No please no global boxes. We need valid parenting.

I assume this is about the
+ { ATOM_colr,    MP4_ReadBox_colr,         0 },
line?
I'm not sure I can guarantee what the parent type will be. So far I've 
only seen it as a 'ap4h' child, but I won't swear it's always the case.
I'm open to opinions/suggestions

>
>> +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.
>

Fair enough, I don't think ncl* are atoms per-se, but my mp4 terminology 
might not be correct.
I'll remove the ATOM_ncl* & will remove the union

> Francois
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel
>

Thanks