diff mbox

[FFmpeg-devel,3/4] avformat/mov: add support for reading Mastering Display Metadata Box

Message ID 20170518004941.5140-3-jamrial@gmail.com
State Accepted
Headers show

Commit Message

James Almer May 18, 2017, 12:49 a.m. UTC 
As defined in "VP Codec ISO Media File Format Binding v1.0"
https://github.com/webmproject/vp9-dash/blob/master/VPCodecISOMediaFileFormatBinding.md

Partially based on Matroska decoder code.

Signed-off-by: James Almer <jamrial@gmail.com>
---
 libavformat/isom.h |  2 ++
 libavformat/mov.c  | 65 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 67 insertions(+)

Comments

Michael Niedermayer May 26, 2017, 11:05 p.m. UTC | #1 
On Wed, May 17, 2017 at 09:49:40PM -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
> 
> Partially based on Matroska decoder code.
> 
> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
>  libavformat/isom.h |  2 ++
>  libavformat/mov.c  | 65 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 67 insertions(+)
> 
> diff --git a/libavformat/isom.h b/libavformat/isom.h
> index d9956cf63a..426f732247 100644
> --- a/libavformat/isom.h
> +++ b/libavformat/isom.h
> @@ -27,6 +27,7 @@
>  #include <stddef.h>
>  #include <stdint.h>
>  
> +#include "libavutil/mastering_display_metadata.h"
>  #include "libavutil/spherical.h"
>  #include "libavutil/stereo3d.h"
>  
> @@ -194,6 +195,7 @@ typedef struct MOVStreamContext {
>      AVStereo3D *stereo3d;
>      AVSphericalMapping *spherical;
>      size_t spherical_size;
> +    AVMasteringDisplayMetadata *mastering;
>  
>      uint32_t format;
>  
> diff --git a/libavformat/mov.c b/libavformat/mov.c
> index afef53b79a..0b5fd849f3 100644
> --- a/libavformat/mov.c
> +++ b/libavformat/mov.c
> @@ -4612,6 +4612,60 @@ static int mov_read_tmcd(MOVContext *c, AVIOContext *pb, MOVAtom atom)
>      return 0;
>  }
>  
> +static int mov_read_smdm(MOVContext *c, AVIOContext *pb, MOVAtom atom)
> +{
> +    MOVStreamContext *sc;
> +    const int chroma_den = 50000;
> +    const int luma_den = 10000;
> +    int version;
> +
> +    if (c->fc->nb_streams < 1)
> +        return AVERROR_INVALIDDATA;
> +
> +    sc = c->fc->streams[c->fc->nb_streams - 1]->priv_data;
> +
> +    if (atom.size < 5) {
> +        av_log(c->fc, AV_LOG_ERROR, "Empty Mastering Display Metadata box\n");
> +        return AVERROR_INVALIDDATA;
> +    }
> +
> +    version = avio_r8(pb);
> +    if (version) {
> +        av_log(c->fc, AV_LOG_WARNING, "Unsupported Mastering Display Metadata box version %d\n", version);
> +        return 0;
> +    }
> +    avio_skip(pb, 3); /* flags */
> +
> +    sc->mastering = av_mastering_display_metadata_alloc();
> +    if (!sc->mastering)
> +        return AVERROR(ENOMEM);
> +

> +    sc->mastering->display_primaries[0][0] =
> +        av_make_q(lrint(((double)avio_rb16(pb) / (1 << 16)) * chroma_den), chroma_den);

this is not optimal, precission wise
av_d2q() should produce closer rationals
alternativly av_reduce() can be used directly

but iam not sure why a fixed chroma_den and luma_den is fixed
maybe iam missing something

[...]
James Almer May 26, 2017, 11:55 p.m. UTC | #2 
On 5/26/2017 8:05 PM, Michael Niedermayer wrote:
> On Wed, May 17, 2017 at 09:49:40PM -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
>>
>> Partially based on Matroska decoder code.
>>
>> Signed-off-by: James Almer <jamrial@gmail.com>
>> ---
>>  libavformat/isom.h |  2 ++
>>  libavformat/mov.c  | 65 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 67 insertions(+)
>>
>> diff --git a/libavformat/isom.h b/libavformat/isom.h
>> index d9956cf63a..426f732247 100644
>> --- a/libavformat/isom.h
>> +++ b/libavformat/isom.h
>> @@ -27,6 +27,7 @@
>>  #include <stddef.h>
>>  #include <stdint.h>
>>  
>> +#include "libavutil/mastering_display_metadata.h"
>>  #include "libavutil/spherical.h"
>>  #include "libavutil/stereo3d.h"
>>  
>> @@ -194,6 +195,7 @@ typedef struct MOVStreamContext {
>>      AVStereo3D *stereo3d;
>>      AVSphericalMapping *spherical;
>>      size_t spherical_size;
>> +    AVMasteringDisplayMetadata *mastering;
>>  
>>      uint32_t format;
>>  
>> diff --git a/libavformat/mov.c b/libavformat/mov.c
>> index afef53b79a..0b5fd849f3 100644
>> --- a/libavformat/mov.c
>> +++ b/libavformat/mov.c
>> @@ -4612,6 +4612,60 @@ static int mov_read_tmcd(MOVContext *c, AVIOContext *pb, MOVAtom atom)
>>      return 0;
>>  }
>>  
>> +static int mov_read_smdm(MOVContext *c, AVIOContext *pb, MOVAtom atom)
>> +{
>> +    MOVStreamContext *sc;
>> +    const int chroma_den = 50000;
>> +    const int luma_den = 10000;
>> +    int version;
>> +
>> +    if (c->fc->nb_streams < 1)
>> +        return AVERROR_INVALIDDATA;
>> +
>> +    sc = c->fc->streams[c->fc->nb_streams - 1]->priv_data;
>> +
>> +    if (atom.size < 5) {
>> +        av_log(c->fc, AV_LOG_ERROR, "Empty Mastering Display Metadata box\n");
>> +        return AVERROR_INVALIDDATA;
>> +    }
>> +
>> +    version = avio_r8(pb);
>> +    if (version) {
>> +        av_log(c->fc, AV_LOG_WARNING, "Unsupported Mastering Display Metadata box version %d\n", version);
>> +        return 0;
>> +    }
>> +    avio_skip(pb, 3); /* flags */
>> +
>> +    sc->mastering = av_mastering_display_metadata_alloc();
>> +    if (!sc->mastering)
>> +        return AVERROR(ENOMEM);
>> +
> 
>> +    sc->mastering->display_primaries[0][0] =
>> +        av_make_q(lrint(((double)avio_rb16(pb) / (1 << 16)) * chroma_den), chroma_den);
> 
> this is not optimal, precission wise
> av_d2q() should produce closer rationals
> alternativly av_reduce() can be used directly
> 
> but iam not sure why a fixed chroma_den and luma_den is fixed
> maybe iam missing something

I took the constants from hevcdec.c and matroskadec.c and basically
copied how they handled these values.
At least based on what lavf's dump.c and mkvtoolnix's mkvinfo reported,
this implementation is the most precise i tried. Using av_d2q() directly
was worse.
James Almer May 27, 2017, 1:14 a.m. UTC | #3 
On 5/26/2017 8:55 PM, James Almer wrote:
> On 5/26/2017 8:05 PM, Michael Niedermayer wrote:
>> On Wed, May 17, 2017 at 09:49:40PM -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
>>>
>>> Partially based on Matroska decoder code.
>>>
>>> Signed-off-by: James Almer <jamrial@gmail.com>
>>> ---
>>>  libavformat/isom.h |  2 ++
>>>  libavformat/mov.c  | 65 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>  2 files changed, 67 insertions(+)
>>>
>>> diff --git a/libavformat/isom.h b/libavformat/isom.h
>>> index d9956cf63a..426f732247 100644
>>> --- a/libavformat/isom.h
>>> +++ b/libavformat/isom.h
>>> @@ -27,6 +27,7 @@
>>>  #include <stddef.h>
>>>  #include <stdint.h>
>>>  
>>> +#include "libavutil/mastering_display_metadata.h"
>>>  #include "libavutil/spherical.h"
>>>  #include "libavutil/stereo3d.h"
>>>  
>>> @@ -194,6 +195,7 @@ typedef struct MOVStreamContext {
>>>      AVStereo3D *stereo3d;
>>>      AVSphericalMapping *spherical;
>>>      size_t spherical_size;
>>> +    AVMasteringDisplayMetadata *mastering;
>>>  
>>>      uint32_t format;
>>>  
>>> diff --git a/libavformat/mov.c b/libavformat/mov.c
>>> index afef53b79a..0b5fd849f3 100644
>>> --- a/libavformat/mov.c
>>> +++ b/libavformat/mov.c
>>> @@ -4612,6 +4612,60 @@ static int mov_read_tmcd(MOVContext *c, AVIOContext *pb, MOVAtom atom)
>>>      return 0;
>>>  }
>>>  
>>> +static int mov_read_smdm(MOVContext *c, AVIOContext *pb, MOVAtom atom)
>>> +{
>>> +    MOVStreamContext *sc;
>>> +    const int chroma_den = 50000;
>>> +    const int luma_den = 10000;
>>> +    int version;
>>> +
>>> +    if (c->fc->nb_streams < 1)
>>> +        return AVERROR_INVALIDDATA;
>>> +
>>> +    sc = c->fc->streams[c->fc->nb_streams - 1]->priv_data;
>>> +
>>> +    if (atom.size < 5) {
>>> +        av_log(c->fc, AV_LOG_ERROR, "Empty Mastering Display Metadata box\n");
>>> +        return AVERROR_INVALIDDATA;
>>> +    }
>>> +
>>> +    version = avio_r8(pb);
>>> +    if (version) {
>>> +        av_log(c->fc, AV_LOG_WARNING, "Unsupported Mastering Display Metadata box version %d\n", version);
>>> +        return 0;
>>> +    }
>>> +    avio_skip(pb, 3); /* flags */
>>> +
>>> +    sc->mastering = av_mastering_display_metadata_alloc();
>>> +    if (!sc->mastering)
>>> +        return AVERROR(ENOMEM);
>>> +
>>
>>> +    sc->mastering->display_primaries[0][0] =
>>> +        av_make_q(lrint(((double)avio_rb16(pb) / (1 << 16)) * chroma_den), chroma_den);
>>
>> this is not optimal, precission wise
>> av_d2q() should produce closer rationals
>> alternativly av_reduce() can be used directly
>>
>> but iam not sure why a fixed chroma_den and luma_den is fixed
>> maybe iam missing something
> 
> I took the constants from hevcdec.c and matroskadec.c and basically
> copied how they handled these values.
> At least based on what lavf's dump.c and mkvtoolnix's mkvinfo reported,
> this implementation is the most precise i tried. Using av_d2q() directly
> was worse.

By dump.c i mean i remuxed an mkv file with mastering metadata using the
muxer implementation of this same patchset then compared the reported
values.
By mkvinfo i mean mkv -> mov (using my muxer implementation) -> mkv
roundtrip then compared the double precision values stored in the
original mkv and the new mkv.
James Almer May 27, 2017, 4:54 a.m. UTC | #4 
On 5/26/2017 8:05 PM, Michael Niedermayer wrote:
> On Wed, May 17, 2017 at 09:49:40PM -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
>>
>> Partially based on Matroska decoder code.
>>
>> Signed-off-by: James Almer <jamrial@gmail.com>
>> ---
>>  libavformat/isom.h |  2 ++
>>  libavformat/mov.c  | 65 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 67 insertions(+)
>>
>> diff --git a/libavformat/isom.h b/libavformat/isom.h
>> index d9956cf63a..426f732247 100644
>> --- a/libavformat/isom.h
>> +++ b/libavformat/isom.h
>> @@ -27,6 +27,7 @@
>>  #include <stddef.h>
>>  #include <stdint.h>
>>  
>> +#include "libavutil/mastering_display_metadata.h"
>>  #include "libavutil/spherical.h"
>>  #include "libavutil/stereo3d.h"
>>  
>> @@ -194,6 +195,7 @@ typedef struct MOVStreamContext {
>>      AVStereo3D *stereo3d;
>>      AVSphericalMapping *spherical;
>>      size_t spherical_size;
>> +    AVMasteringDisplayMetadata *mastering;
>>  
>>      uint32_t format;
>>  
>> diff --git a/libavformat/mov.c b/libavformat/mov.c
>> index afef53b79a..0b5fd849f3 100644
>> --- a/libavformat/mov.c
>> +++ b/libavformat/mov.c
>> @@ -4612,6 +4612,60 @@ static int mov_read_tmcd(MOVContext *c, AVIOContext *pb, MOVAtom atom)
>>      return 0;
>>  }
>>  
>> +static int mov_read_smdm(MOVContext *c, AVIOContext *pb, MOVAtom atom)
>> +{
>> +    MOVStreamContext *sc;
>> +    const int chroma_den = 50000;
>> +    const int luma_den = 10000;
>> +    int version;
>> +
>> +    if (c->fc->nb_streams < 1)
>> +        return AVERROR_INVALIDDATA;
>> +
>> +    sc = c->fc->streams[c->fc->nb_streams - 1]->priv_data;
>> +
>> +    if (atom.size < 5) {
>> +        av_log(c->fc, AV_LOG_ERROR, "Empty Mastering Display Metadata box\n");
>> +        return AVERROR_INVALIDDATA;
>> +    }
>> +
>> +    version = avio_r8(pb);
>> +    if (version) {
>> +        av_log(c->fc, AV_LOG_WARNING, "Unsupported Mastering Display Metadata box version %d\n", version);
>> +        return 0;
>> +    }
>> +    avio_skip(pb, 3); /* flags */
>> +
>> +    sc->mastering = av_mastering_display_metadata_alloc();
>> +    if (!sc->mastering)
>> +        return AVERROR(ENOMEM);
>> +
> 
>> +    sc->mastering->display_primaries[0][0] =
>> +        av_make_q(lrint(((double)avio_rb16(pb) / (1 << 16)) * chroma_den), chroma_den);
> 
> this is not optimal, precission wise
> av_d2q() should produce closer rationals
> alternativly av_reduce() can be used directly
> 
> but iam not sure why a fixed chroma_den and luma_den is fixed
> maybe iam missing something

Does

for (i = 0; i < 3; i++)
  for (j = 0; j < 2; j++)
    av_reduce(&sc->mastering->display_primaries[i][j].num,
              &sc->mastering->display_primaries[i][j].den,
              lrint(((double)avio_rb16(pb) / (1 << 16)) * chroma_den),
              chroma_den, chroma_den);
for (i = 0; i < 2; i++)
  av_reduce(&sc->mastering->white_point[i].num,
            &sc->mastering->white_point[i].den,
            lrint(((double)avio_rb16(pb) / (1 << 16)) * chroma_den),
            chroma_den, chroma_den);
av_reduce(&sc->mastering->max_luminance.num,
          &sc->mastering->max_luminance.den,
          lrint(((double)avio_rb32(pb) / (1 << 8)) * luma_den),
          luma_den, INT_MAX);
av_reduce(&sc->mastering->min_luminance.num,
          &sc->mastering->min_luminance.den,
          lrint(((double)avio_rb32(pb) / (1 << 14)) * luma_den),
          luma_den, INT_MAX);

Look better?
Michael Niedermayer May 27, 2017, 2:05 p.m. UTC | #5 
On Sat, May 27, 2017 at 01:54:18AM -0300, James Almer wrote:
> On 5/26/2017 8:05 PM, Michael Niedermayer wrote:
> > On Wed, May 17, 2017 at 09:49:40PM -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
> >>
> >> Partially based on Matroska decoder code.
> >>
> >> Signed-off-by: James Almer <jamrial@gmail.com>
> >> ---
> >>  libavformat/isom.h |  2 ++
> >>  libavformat/mov.c  | 65 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >>  2 files changed, 67 insertions(+)
> >>
> >> diff --git a/libavformat/isom.h b/libavformat/isom.h
> >> index d9956cf63a..426f732247 100644
> >> --- a/libavformat/isom.h
> >> +++ b/libavformat/isom.h
> >> @@ -27,6 +27,7 @@
> >>  #include <stddef.h>
> >>  #include <stdint.h>
> >>  
> >> +#include "libavutil/mastering_display_metadata.h"
> >>  #include "libavutil/spherical.h"
> >>  #include "libavutil/stereo3d.h"
> >>  
> >> @@ -194,6 +195,7 @@ typedef struct MOVStreamContext {
> >>      AVStereo3D *stereo3d;
> >>      AVSphericalMapping *spherical;
> >>      size_t spherical_size;
> >> +    AVMasteringDisplayMetadata *mastering;
> >>  
> >>      uint32_t format;
> >>  
> >> diff --git a/libavformat/mov.c b/libavformat/mov.c
> >> index afef53b79a..0b5fd849f3 100644
> >> --- a/libavformat/mov.c
> >> +++ b/libavformat/mov.c
> >> @@ -4612,6 +4612,60 @@ static int mov_read_tmcd(MOVContext *c, AVIOContext *pb, MOVAtom atom)
> >>      return 0;
> >>  }
> >>  
> >> +static int mov_read_smdm(MOVContext *c, AVIOContext *pb, MOVAtom atom)
> >> +{
> >> +    MOVStreamContext *sc;
> >> +    const int chroma_den = 50000;
> >> +    const int luma_den = 10000;
> >> +    int version;
> >> +
> >> +    if (c->fc->nb_streams < 1)
> >> +        return AVERROR_INVALIDDATA;
> >> +
> >> +    sc = c->fc->streams[c->fc->nb_streams - 1]->priv_data;
> >> +
> >> +    if (atom.size < 5) {
> >> +        av_log(c->fc, AV_LOG_ERROR, "Empty Mastering Display Metadata box\n");
> >> +        return AVERROR_INVALIDDATA;
> >> +    }
> >> +
> >> +    version = avio_r8(pb);
> >> +    if (version) {
> >> +        av_log(c->fc, AV_LOG_WARNING, "Unsupported Mastering Display Metadata box version %d\n", version);
> >> +        return 0;
> >> +    }
> >> +    avio_skip(pb, 3); /* flags */
> >> +
> >> +    sc->mastering = av_mastering_display_metadata_alloc();
> >> +    if (!sc->mastering)
> >> +        return AVERROR(ENOMEM);
> >> +
> > 
> >> +    sc->mastering->display_primaries[0][0] =
> >> +        av_make_q(lrint(((double)avio_rb16(pb) / (1 << 16)) * chroma_den), chroma_den);
> > 
> > this is not optimal, precission wise
> > av_d2q() should produce closer rationals
> > alternativly av_reduce() can be used directly
> > 
> > but iam not sure why a fixed chroma_den and luma_den is fixed
> > maybe iam missing something
> 
> Does
> 
> for (i = 0; i < 3; i++)
>   for (j = 0; j < 2; j++)
>     av_reduce(&sc->mastering->display_primaries[i][j].num,
>               &sc->mastering->display_primaries[i][j].den,
>               lrint(((double)avio_rb16(pb) / (1 << 16)) * chroma_den),
>               chroma_den, chroma_den);

Why do you use
lrint(((double)avio_rb16(pb) / (1 << 16)) * chroma_den
and
chroma_den

instead of
avio_rb16(pb)
and
1 << 16

?

Is the intend to guess the value prior to muxing instead of using
the value actually stored in the mov file ?


[...]
James Almer May 27, 2017, 2:58 p.m. UTC | #6 
On 5/27/2017 11:05 AM, Michael Niedermayer wrote:
> On Sat, May 27, 2017 at 01:54:18AM -0300, James Almer wrote:
>> On 5/26/2017 8:05 PM, Michael Niedermayer wrote:
>>> On Wed, May 17, 2017 at 09:49:40PM -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
>>>>
>>>> Partially based on Matroska decoder code.
>>>>
>>>> Signed-off-by: James Almer <jamrial@gmail.com>
>>>> ---
>>>>  libavformat/isom.h |  2 ++
>>>>  libavformat/mov.c  | 65 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>  2 files changed, 67 insertions(+)
>>>>
>>>> diff --git a/libavformat/isom.h b/libavformat/isom.h
>>>> index d9956cf63a..426f732247 100644
>>>> --- a/libavformat/isom.h
>>>> +++ b/libavformat/isom.h
>>>> @@ -27,6 +27,7 @@
>>>>  #include <stddef.h>
>>>>  #include <stdint.h>
>>>>  
>>>> +#include "libavutil/mastering_display_metadata.h"
>>>>  #include "libavutil/spherical.h"
>>>>  #include "libavutil/stereo3d.h"
>>>>  
>>>> @@ -194,6 +195,7 @@ typedef struct MOVStreamContext {
>>>>      AVStereo3D *stereo3d;
>>>>      AVSphericalMapping *spherical;
>>>>      size_t spherical_size;
>>>> +    AVMasteringDisplayMetadata *mastering;
>>>>  
>>>>      uint32_t format;
>>>>  
>>>> diff --git a/libavformat/mov.c b/libavformat/mov.c
>>>> index afef53b79a..0b5fd849f3 100644
>>>> --- a/libavformat/mov.c
>>>> +++ b/libavformat/mov.c
>>>> @@ -4612,6 +4612,60 @@ static int mov_read_tmcd(MOVContext *c, AVIOContext *pb, MOVAtom atom)
>>>>      return 0;
>>>>  }
>>>>  
>>>> +static int mov_read_smdm(MOVContext *c, AVIOContext *pb, MOVAtom atom)
>>>> +{
>>>> +    MOVStreamContext *sc;
>>>> +    const int chroma_den = 50000;
>>>> +    const int luma_den = 10000;
>>>> +    int version;
>>>> +
>>>> +    if (c->fc->nb_streams < 1)
>>>> +        return AVERROR_INVALIDDATA;
>>>> +
>>>> +    sc = c->fc->streams[c->fc->nb_streams - 1]->priv_data;
>>>> +
>>>> +    if (atom.size < 5) {
>>>> +        av_log(c->fc, AV_LOG_ERROR, "Empty Mastering Display Metadata box\n");
>>>> +        return AVERROR_INVALIDDATA;
>>>> +    }
>>>> +
>>>> +    version = avio_r8(pb);
>>>> +    if (version) {
>>>> +        av_log(c->fc, AV_LOG_WARNING, "Unsupported Mastering Display Metadata box version %d\n", version);
>>>> +        return 0;
>>>> +    }
>>>> +    avio_skip(pb, 3); /* flags */
>>>> +
>>>> +    sc->mastering = av_mastering_display_metadata_alloc();
>>>> +    if (!sc->mastering)
>>>> +        return AVERROR(ENOMEM);
>>>> +
>>>
>>>> +    sc->mastering->display_primaries[0][0] =
>>>> +        av_make_q(lrint(((double)avio_rb16(pb) / (1 << 16)) * chroma_den), chroma_den);
>>>
>>> this is not optimal, precission wise
>>> av_d2q() should produce closer rationals
>>> alternativly av_reduce() can be used directly
>>>
>>> but iam not sure why a fixed chroma_den and luma_den is fixed
>>> maybe iam missing something
>>
>> Does
>>
>> for (i = 0; i < 3; i++)
>>   for (j = 0; j < 2; j++)
>>     av_reduce(&sc->mastering->display_primaries[i][j].num,
>>               &sc->mastering->display_primaries[i][j].den,
>>               lrint(((double)avio_rb16(pb) / (1 << 16)) * chroma_den),
>>               chroma_den, chroma_den);
> 
> Why do you use
> lrint(((double)avio_rb16(pb) / (1 << 16)) * chroma_den
> and
> chroma_den
> 
> instead of
> avio_rb16(pb)
> and
> 1 << 16
> 
> ?
> 

To follow the origin spec the vp9 in mp4 spec and the
AVMasteringDisplayMetadata API quote and are based on.

---------------
display_primaries_x[ c ] and display_primaries_y[ c ] specify the
normalized x and y chromaticity coordinates, respectively, of the colour
primary component c of the mastering display in increments of 0.00002,
according to the CIE 1931 definition of x and y as specified in ISO
11664-1 (see also ISO 11664-3 and CIE 15). For describing mastering
displays that use red, green and blue colour primaries, it is suggested
that index value c equal to 0 should correspond to the green primary, c
equal to 1 should correspond to the blue primary and c equal to 2 should
correspond to the red colour primary (see also Annex E and Table E.3).
The values of display_primaries_x[ c ] and display_primaries_y[ c ]
shall be in the range of 0 to 50 000, inclusive.

white_point_x and white_point_y specify the normalized x and y
chromaticity coordinates, respectively, of the white point of the
mastering display in normalized increments of 0.00002, according to the
CIE 1931 definition of x and y as specified in ISO 11664-1 (see also ISO
11664-3 and CIE 15). The values of white_point_x and white_point_y shall
be in the range of 0 to 50 000.

max_display_mastering_luminance and min_display_mastering_luminance
specify the nominal maximum and minimum display luminance, respectively,
of the mastering display in units of 0.0001 candelas per square metre.
------------

$ ./ffmpeg -i The\ World\ in\ HDR-tO01J-M3g0U.webm
    Side data:
      Mastering Display Metadata, has_primaries:1 has_luminance:1
r(0.6800,0.3200) g(0.2649,0.6900) b(0.1500 0.0600) wp(0.3127, 0.3290)
min_luminance=0.001000, max_luminance=1000.000000

$ mkvinfo The\ World\ in\ HDR-tO01J-M3g0U.webm
|    + Video colour mastering metadata
|     + Max luminance: 1000
|     + Min luminance: 0.001
|     + Red colour coordinate x: 0.68
|     + Red colour coordinate y: 0.31996
|     + Green colour coordinate x: 0.26494
|     + Green colour coordinate y: 0.68996
|     + Blue colour coordinate x: 0.15
|     + Blue colour coordinate y: 0.05998
|     + White colour coordinate x: 0.3127
|     + White colour coordinate y: 0.32896

$ ./ffmpeg -i The\ World\ in\ HDR-tO01J-M3g0U.webm -c:v copy mastering.mp4

------

Without chroma/luma_den rounding in demuxer

$ ./ffmpeg -i mastering.mp4
    Side data:
      Mastering Display Metadata, has_primaries:1 has_luminance:1
r(0.6800,0.3200) g(0.2649,0.6900) b(0.1500 0.0600) wp(0.3127, 0.3290)
min_luminance=0.000977, max_luminance=1000.000000

$ ./ffmpeg -i mastering.mp4 -c:v copy without_den.mkv

$ mkvinfo without_den.mkv
|    + Video colour mastering metadata
|     + Red colour coordinate x: 0.679993
|     + Red colour coordinate y: 0.319962
|     + Green colour coordinate x: 0.264938
|     + Green colour coordinate y: 0.689957
|     + Blue colour coordinate x: 0.149994
|     + Blue colour coordinate y: 0.0599823
|     + White colour coordinate x: 0.312698
|     + White colour coordinate y: 0.328964
|     + Max luminance: 1000
|     + Min luminance: 0.000976562

------

With chroma/luma_den rounding in demuxer

$ ./ffmpeg -i mastering.mp4
    Side data:
      Mastering Display Metadata, has_primaries:1 has_luminance:1
r(0.6800,0.3200) g(0.2649,0.6900) b(0.1500 0.0600) wp(0.3127, 0.3290)
min_luminance=0.001000, max_luminance=1000.000000

$ ./ffmpeg -i mastering.mp4 -c:v copy with_den.mkv

$ mkvinfo with_den.mkv
|     + Red colour coordinate x: 0.68
|     + Red colour coordinate y: 0.31996
|     + Green colour coordinate x: 0.26494
|     + Green colour coordinate y: 0.68996
|     + Blue colour coordinate x: 0.15
|     + Blue colour coordinate y: 0.05998
|     + White colour coordinate x: 0.3127
|     + White colour coordinate y: 0.32896
|     + Max luminance: 1000
|     + Min luminance: 0.001

------

Notice how all values break the increments of 0.00002 (50000
denominator) and 0.0001 (10000 denominator) constrains when i just take
the 0.16, 24.8 and 18.14 fixed point values stored in mp4 and dump them
into AVMasteringDisplayMetadata as is, like you suggest.
Michael Niedermayer May 27, 2017, 4:12 p.m. UTC | #7 
On Sat, May 27, 2017 at 11:58:05AM -0300, James Almer wrote:
> On 5/27/2017 11:05 AM, Michael Niedermayer wrote:
> > On Sat, May 27, 2017 at 01:54:18AM -0300, James Almer wrote:
> >> On 5/26/2017 8:05 PM, Michael Niedermayer wrote:
> >>> On Wed, May 17, 2017 at 09:49:40PM -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
> >>>>
> >>>> Partially based on Matroska decoder code.
> >>>>
> >>>> Signed-off-by: James Almer <jamrial@gmail.com>
> >>>> ---
> >>>>  libavformat/isom.h |  2 ++
> >>>>  libavformat/mov.c  | 65 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >>>>  2 files changed, 67 insertions(+)
> >>>>
> >>>> diff --git a/libavformat/isom.h b/libavformat/isom.h
> >>>> index d9956cf63a..426f732247 100644
> >>>> --- a/libavformat/isom.h
> >>>> +++ b/libavformat/isom.h
> >>>> @@ -27,6 +27,7 @@
> >>>>  #include <stddef.h>
> >>>>  #include <stdint.h>
> >>>>  
> >>>> +#include "libavutil/mastering_display_metadata.h"
> >>>>  #include "libavutil/spherical.h"
> >>>>  #include "libavutil/stereo3d.h"
> >>>>  
> >>>> @@ -194,6 +195,7 @@ typedef struct MOVStreamContext {
> >>>>      AVStereo3D *stereo3d;
> >>>>      AVSphericalMapping *spherical;
> >>>>      size_t spherical_size;
> >>>> +    AVMasteringDisplayMetadata *mastering;
> >>>>  
> >>>>      uint32_t format;
> >>>>  
> >>>> diff --git a/libavformat/mov.c b/libavformat/mov.c
> >>>> index afef53b79a..0b5fd849f3 100644
> >>>> --- a/libavformat/mov.c
> >>>> +++ b/libavformat/mov.c
> >>>> @@ -4612,6 +4612,60 @@ static int mov_read_tmcd(MOVContext *c, AVIOContext *pb, MOVAtom atom)
> >>>>      return 0;
> >>>>  }
> >>>>  
> >>>> +static int mov_read_smdm(MOVContext *c, AVIOContext *pb, MOVAtom atom)
> >>>> +{
> >>>> +    MOVStreamContext *sc;
> >>>> +    const int chroma_den = 50000;
> >>>> +    const int luma_den = 10000;
> >>>> +    int version;
> >>>> +
> >>>> +    if (c->fc->nb_streams < 1)
> >>>> +        return AVERROR_INVALIDDATA;
> >>>> +
> >>>> +    sc = c->fc->streams[c->fc->nb_streams - 1]->priv_data;
> >>>> +
> >>>> +    if (atom.size < 5) {
> >>>> +        av_log(c->fc, AV_LOG_ERROR, "Empty Mastering Display Metadata box\n");
> >>>> +        return AVERROR_INVALIDDATA;
> >>>> +    }
> >>>> +
> >>>> +    version = avio_r8(pb);
> >>>> +    if (version) {
> >>>> +        av_log(c->fc, AV_LOG_WARNING, "Unsupported Mastering Display Metadata box version %d\n", version);
> >>>> +        return 0;
> >>>> +    }
> >>>> +    avio_skip(pb, 3); /* flags */
> >>>> +
> >>>> +    sc->mastering = av_mastering_display_metadata_alloc();
> >>>> +    if (!sc->mastering)
> >>>> +        return AVERROR(ENOMEM);
> >>>> +
> >>>
> >>>> +    sc->mastering->display_primaries[0][0] =
> >>>> +        av_make_q(lrint(((double)avio_rb16(pb) / (1 << 16)) * chroma_den), chroma_den);
> >>>
> >>> this is not optimal, precission wise
> >>> av_d2q() should produce closer rationals
> >>> alternativly av_reduce() can be used directly
> >>>
> >>> but iam not sure why a fixed chroma_den and luma_den is fixed
> >>> maybe iam missing something
> >>
> >> Does
> >>
> >> for (i = 0; i < 3; i++)
> >>   for (j = 0; j < 2; j++)
> >>     av_reduce(&sc->mastering->display_primaries[i][j].num,
> >>               &sc->mastering->display_primaries[i][j].den,
> >>               lrint(((double)avio_rb16(pb) / (1 << 16)) * chroma_den),
> >>               chroma_den, chroma_den);
> > 
> > Why do you use
> > lrint(((double)avio_rb16(pb) / (1 << 16)) * chroma_den
> > and
> > chroma_den
> > 
> > instead of
> > avio_rb16(pb)
> > and
> > 1 << 16
> > 
> > ?
> > 
> 
> To follow the origin spec the vp9 in mp4 spec and the
> AVMasteringDisplayMetadata API quote and are based on.
> 
> ---------------
> display_primaries_x[ c ] and display_primaries_y[ c ] specify the
> normalized x and y chromaticity coordinates, respectively, of the colour
> primary component c of the mastering display in increments of 0.00002,
> according to the CIE 1931 definition of x and y as specified in ISO
> 11664-1 (see also ISO 11664-3 and CIE 15). For describing mastering
> displays that use red, green and blue colour primaries, it is suggested
> that index value c equal to 0 should correspond to the green primary, c
> equal to 1 should correspond to the blue primary and c equal to 2 should
> correspond to the red colour primary (see also Annex E and Table E.3).
> The values of display_primaries_x[ c ] and display_primaries_y[ c ]
> shall be in the range of 0 to 50 000, inclusive.
> 
> white_point_x and white_point_y specify the normalized x and y
> chromaticity coordinates, respectively, of the white point of the
> mastering display in normalized increments of 0.00002, according to the
> CIE 1931 definition of x and y as specified in ISO 11664-1 (see also ISO
> 11664-3 and CIE 15). The values of white_point_x and white_point_y shall
> be in the range of 0 to 50 000.
> 
> max_display_mastering_luminance and min_display_mastering_luminance
> specify the nominal maximum and minimum display luminance, respectively,
> of the mastering display in units of 0.0001 candelas per square metre.

[...]

> Notice how all values break the increments of 0.00002 (50000
> denominator) and 0.0001 (10000 denominator) constrains when i just take
> the 0.16, 24.8 and 18.14 fixed point values stored in mp4 and dump them
> into AVMasteringDisplayMetadata as is, like you suggest.

ok, i mistakely assumed the specifications where consistent or at
least one a superset of others.

Please ignore my comment about the patch


[...]
James Almer May 27, 2017, 7:16 p.m. UTC | #8 
On 5/27/2017 1:12 PM, Michael Niedermayer wrote:
> ok, i mistakely assumed the specifications where consistent or at
> least one a superset of others.
> 
> Please ignore my comment about the patch

Pushed then. Thanks.
diff mbox

Patch

diff --git a/libavformat/isom.h b/libavformat/isom.h
index d9956cf63a..426f732247 100644
--- a/libavformat/isom.h
+++ b/libavformat/isom.h
@@ -27,6 +27,7 @@ 
 #include <stddef.h>
 #include <stdint.h>
 
+#include "libavutil/mastering_display_metadata.h"
 #include "libavutil/spherical.h"
 #include "libavutil/stereo3d.h"
 
@@ -194,6 +195,7 @@  typedef struct MOVStreamContext {
     AVStereo3D *stereo3d;
     AVSphericalMapping *spherical;
     size_t spherical_size;
+    AVMasteringDisplayMetadata *mastering;
 
     uint32_t format;
 
diff --git a/libavformat/mov.c b/libavformat/mov.c
index afef53b79a..0b5fd849f3 100644
--- a/libavformat/mov.c
+++ b/libavformat/mov.c
@@ -4612,6 +4612,60 @@  static int mov_read_tmcd(MOVContext *c, AVIOContext *pb, MOVAtom atom)
     return 0;
 }
 
+static int mov_read_smdm(MOVContext *c, AVIOContext *pb, MOVAtom atom)
+{
+    MOVStreamContext *sc;
+    const int chroma_den = 50000;
+    const int luma_den = 10000;
+    int version;
+
+    if (c->fc->nb_streams < 1)
+        return AVERROR_INVALIDDATA;
+
+    sc = c->fc->streams[c->fc->nb_streams - 1]->priv_data;
+
+    if (atom.size < 5) {
+        av_log(c->fc, AV_LOG_ERROR, "Empty Mastering Display Metadata box\n");
+        return AVERROR_INVALIDDATA;
+    }
+
+    version = avio_r8(pb);
+    if (version) {
+        av_log(c->fc, AV_LOG_WARNING, "Unsupported Mastering Display Metadata box version %d\n", version);
+        return 0;
+    }
+    avio_skip(pb, 3); /* flags */
+
+    sc->mastering = av_mastering_display_metadata_alloc();
+    if (!sc->mastering)
+        return AVERROR(ENOMEM);
+
+    sc->mastering->display_primaries[0][0] =
+        av_make_q(lrint(((double)avio_rb16(pb) / (1 << 16)) * chroma_den), chroma_den);
+    sc->mastering->display_primaries[0][1] =
+        av_make_q(lrint(((double)avio_rb16(pb) / (1 << 16)) * chroma_den), chroma_den);
+    sc->mastering->display_primaries[1][0] =
+        av_make_q(lrint(((double)avio_rb16(pb) / (1 << 16)) * chroma_den), chroma_den);
+    sc->mastering->display_primaries[1][1] =
+        av_make_q(lrint(((double)avio_rb16(pb) / (1 << 16)) * chroma_den), chroma_den);
+    sc->mastering->display_primaries[2][0] =
+        av_make_q(lrint(((double)avio_rb16(pb) / (1 << 16)) * chroma_den), chroma_den);
+    sc->mastering->display_primaries[2][1] =
+        av_make_q(lrint(((double)avio_rb16(pb) / (1 << 16)) * chroma_den), chroma_den);
+    sc->mastering->white_point[0]          =
+        av_make_q(lrint(((double)avio_rb16(pb) / (1 << 16)) * chroma_den), chroma_den);
+    sc->mastering->white_point[1]          =
+        av_make_q(lrint(((double)avio_rb16(pb) / (1 << 16)) * chroma_den), chroma_den);
+    sc->mastering->max_luminance           =
+        av_make_q(lrint(((double)avio_rb32(pb) / (1 <<  8)) *   luma_den),   luma_den);
+    sc->mastering->min_luminance           =
+        av_make_q(lrint(((double)avio_rb32(pb) / (1 << 14)) *   luma_den),   luma_den);
+    sc->mastering->has_primaries = 1;
+    sc->mastering->has_luminance = 1;
+
+    return 0;
+}
+
 static int mov_read_st3d(MOVContext *c, AVIOContext *pb, MOVAtom atom)
 {
     AVStream *st;
@@ -5398,6 +5452,7 @@  static const MOVParseTableEntry mov_default_parse_table[] = {
 { MKTAG('s','t','3','d'), mov_read_st3d }, /* stereoscopic 3D video box */
 { MKTAG('s','v','3','d'), mov_read_sv3d }, /* spherical video box */
 { MKTAG('d','O','p','s'), mov_read_dops },
+{ MKTAG('S','m','D','m'), mov_read_smdm },
 { 0, NULL }
 };
 
@@ -5822,6 +5877,7 @@  static int mov_read_close(AVFormatContext *s)
 
         av_freep(&sc->stereo3d);
         av_freep(&sc->spherical);
+        av_freep(&sc->mastering);
     }
 
     if (mov->dv_demux) {
@@ -6172,6 +6228,15 @@  static int mov_read_header(AVFormatContext *s)
 
                 sc->spherical = NULL;
             }
+            if (sc->mastering) {
+                err = av_stream_add_side_data(st, AV_PKT_DATA_MASTERING_DISPLAY_METADATA,
+                                              (uint8_t *)sc->mastering,
+                                              sizeof(*sc->mastering));
+                if (err < 0)
+                    return err;
+
+                sc->mastering = NULL;
+            }
             break;
         }
     }