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

Submitted by James Almer on May 18, 2017, 12:49 a.m.

Details

Message ID 20170518004941.5140-2-jamrial@gmail.com
State New
Headers show

Commit Message

James Almer May 18, 2017, 12:49 a.m.
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@gmail.com>
---
 libavformat/movenc.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

Comments

Michael Niedermayer May 26, 2017, 10:56 p.m.
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@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 not then this can be too small and crash


[...]
James Almer May 26, 2017, 11:58 p.m.
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@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.

Patch hide | download patch | download mbox

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;
+
+    pos = avio_tell(pb);
+
+    avio_wb32(pb, 0);
+    ffio_wfourcc(pb, "CoLL");
+    avio_wb32(pb, 0); /* version & flags */
+    avio_wb16(pb, coll->MaxCLL);
+    avio_wb16(pb, coll->MaxFALL);
+    return update_size(pb, pos);
+}
+
 static int mov_write_hvcc_tag(AVIOContext *pb, MOVTrack *track)
 {
     int64_t pos = avio_tell(pb);
@@ -2001,6 +2022,7 @@  static int mov_write_video_tag(AVIOContext *pb, MOVMuxContext *mov, MOVTrack *tr
     } else if (track->par->codec_id == AV_CODEC_ID_VP9) {
         mov_write_vpcc_tag(mov->fc, pb, track);
         mov_write_smdm_tag(mov->fc, pb, track);
+        mov_write_coll_tag(pb, track);
     } else if (track->par->codec_id == AV_CODEC_ID_VC1 && track->vos_len > 0)
         mov_write_dvc1_tag(pb, track);
     else if (track->par->codec_id == AV_CODEC_ID_VP6F ||