[FFmpeg-devel] avformat/matroskadec: use mastering display metadata's own alloc function

Signed-off-by: James Almer <jamrial@gmail.com>
---
 libavformat/matroskadec.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

On Fri,  2 Dec 2016 16:09:25 -0300
James Almer <jamrial@gmail.com> wrote:

> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
>  libavformat/matroskadec.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
> index 017a533..7b070ff 100644
> --- a/libavformat/matroskadec.c
> +++ b/libavformat/matroskadec.c
> @@ -1841,14 +1841,11 @@ static int mkv_parse_video_color(AVStream *st, const MatroskaTrack *track) {
>          // Use similar rationals as other standards.
>          const int chroma_den = 50000;
>          const int luma_den = 10000;
> -        AVMasteringDisplayMetadata *metadata =
> -            (AVMasteringDisplayMetadata*) av_stream_new_side_data(
> -                st, AV_PKT_DATA_MASTERING_DISPLAY_METADATA,
> -                sizeof(AVMasteringDisplayMetadata));
> +        int ret;
> +        AVMasteringDisplayMetadata *metadata = av_mastering_display_metadata_alloc();
>          if (!metadata) {
>              return AVERROR(ENOMEM);
>          }
> -        memset(metadata, 0, sizeof(AVMasteringDisplayMetadata));

ok...

>          if (has_mastering_primaries) {
>              metadata->display_primaries[0][0] = av_make_q(
>                  round(mastering_meta->r_x * chroma_den), chroma_den);
> @@ -1875,6 +1872,13 @@ static int mkv_parse_video_color(AVStream *st, const MatroskaTrack *track) {
>                  round(mastering_meta->min_luminance * luma_den), luma_den);
>              metadata->has_luminance = 1;
>          }
> +
> +        ret = av_stream_add_side_data(st, AV_PKT_DATA_MASTERING_DISPLAY_METADATA,
> +                                      (uint8_t *)metadata, sizeof(AVMasteringDisplayMetadata));
> +        if (ret < 0) {
> +            av_freep(&metadata);
> +            return ret;
> +        }
>      }
>      return 0;
>  }

Uh what? Am I missing something, or do you rely on
sizeof(AVMasteringDisplayMetadata) being part of the ABI? Because the
alloc function only exists because the struct size is not part of the
ABI.

On 12/2/2016 4:43 PM, wm4 wrote:
> On Fri,  2 Dec 2016 16:09:25 -0300
> James Almer <jamrial@gmail.com> wrote:
> 
>> Signed-off-by: James Almer <jamrial@gmail.com>
>> ---
>>  libavformat/matroskadec.c | 14 +++++++++-----
>>  1 file changed, 9 insertions(+), 5 deletions(-)
>>
>> diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
>> index 017a533..7b070ff 100644
>> --- a/libavformat/matroskadec.c
>> +++ b/libavformat/matroskadec.c
>> @@ -1841,14 +1841,11 @@ static int mkv_parse_video_color(AVStream *st, const MatroskaTrack *track) {
>>          // Use similar rationals as other standards.
>>          const int chroma_den = 50000;
>>          const int luma_den = 10000;
>> -        AVMasteringDisplayMetadata *metadata =
>> -            (AVMasteringDisplayMetadata*) av_stream_new_side_data(
>> -                st, AV_PKT_DATA_MASTERING_DISPLAY_METADATA,
>> -                sizeof(AVMasteringDisplayMetadata));
>> +        int ret;
>> +        AVMasteringDisplayMetadata *metadata = av_mastering_display_metadata_alloc();
>>          if (!metadata) {
>>              return AVERROR(ENOMEM);
>>          }
>> -        memset(metadata, 0, sizeof(AVMasteringDisplayMetadata));
> 
> ok...
> 
>>          if (has_mastering_primaries) {
>>              metadata->display_primaries[0][0] = av_make_q(
>>                  round(mastering_meta->r_x * chroma_den), chroma_den);
>> @@ -1875,6 +1872,13 @@ static int mkv_parse_video_color(AVStream *st, const MatroskaTrack *track) {
>>                  round(mastering_meta->min_luminance * luma_den), luma_den);
>>              metadata->has_luminance = 1;
>>          }
>> +
>> +        ret = av_stream_add_side_data(st, AV_PKT_DATA_MASTERING_DISPLAY_METADATA,
>> +                                      (uint8_t *)metadata, sizeof(AVMasteringDisplayMetadata));
>> +        if (ret < 0) {
>> +            av_freep(&metadata);
>> +            return ret;
>> +        }
>>      }
>>      return 0;
>>  }
> 
> Uh what? Am I missing something, or do you rely on
> sizeof(AVMasteringDisplayMetadata) being part of the ABI? Because the
> alloc function only exists because the struct size is not part of the
> ABI.

I know, i mentioned as much in another email. Notice how sizeof() is
already being used above, in the code I'm removing.

This is mostly cosmetic, until either the size of the struct is defined
as public or a new function that returns said size is introduced.