[FFmpeg-devel] [PATCH 2/4] avformat/movenc: add support for writing Content Light Level Box

[James Almer]

On 5/26/2017 7:56 PM, Michael Niedermayer wrote:
> On Wed, May 17, 2017 at 09:49:39PM -0300, James Almer wrote:
>> As defined in "VP Codec ISO Media File Format Binding v1.0"
>> https://github.com/webmproject/vp9-dash/blob/master/VPCodecISOMediaFileFormatBinding.md
>>
>> Signed-off-by: James Almer <jamrial at gmail.com>
>> ---
>>  libavformat/movenc.c | 22 ++++++++++++++++++++++
>>  1 file changed, 22 insertions(+)
>>
>> diff --git a/libavformat/movenc.c b/libavformat/movenc.c
>> index cd436df7a4..eab7bbc8a7 100644
>> --- a/libavformat/movenc.c
>> +++ b/libavformat/movenc.c
>> @@ -1154,6 +1154,27 @@ static int mov_write_smdm_tag(AVFormatContext *s, AVIOContext *pb, MOVTrack *tra
>>      return update_size(pb, pos);
>>  }
>>  
>> +static int mov_write_coll_tag(AVIOContext *pb, MOVTrack *track)
>> +{
>> +    int size = 0;
>> +    int64_t pos;
>> +    const AVContentLightMetadata *coll =
>> +        (const AVContentLightMetadata *) av_stream_get_side_data(track->st,
>> +                                             AV_PKT_DATA_CONTENT_LIGHT_LEVEL,
>> +                                             &size);
>> +    if (!size)
>> +        return 0;
> 
> Is there anything that checks the validity of size for a
> AVContentLightMetadata ?
> (that is, is this checked anywhere from side data creation to
>  its use here)

If created by av_content_light_metadata_alloc() and the returned size
passed to av_stream_add_side_data(), then it's guaranteed to be correct.
Any other case, guess it depends on if whatever created the side data
ignored the "size not part of the ABI" warning and sizeof'd the struct
or not.

> 
> If not then this can be too small and crash

That's apparently a risk on every current usage of side data (mastering,
stereo3d, spherical, content light, etc) in demuxing code, except those
doing sizeof() against what the doxy says.

I suggested once adding functions that return the size of the struct,
much like the alloc() ones do. Some thought it was a good solution,
others thought it was added complexity to a simple API, and others
wanted to rewrite the entire side data handling code API from scratch.
In the end, none of them were done.