[FFmpeg-devel] matroskaenc: Add support for writing video projection.

Adding support for writing spherical metadata in Matroska.

On 1/27/2017 5:12 PM, Aaron Colwell wrote:
> Adding support for writing spherical metadata in Matroska.
> 
> 
> 0001-matroskaenc-Add-support-for-writing-video-projection.patch
> 
> 
> From 5a9cf56bf3114186412bb5572b153f886edb6ddb Mon Sep 17 00:00:00 2001
> From: Aaron Colwell <acolwell@google.com>
> Date: Fri, 27 Jan 2017 12:07:25 -0800
> Subject: [PATCH] matroskaenc: Add support for writing video projection
>  element.
> 
> Adding support for writing spherical metadata in Matroska.
> ---
>  libavformat/matroskaenc.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 64 insertions(+)
> 
> diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
> index f731b678b9..1b186db223 100644
> --- a/libavformat/matroskaenc.c
> +++ b/libavformat/matroskaenc.c
> @@ -1038,6 +1038,67 @@ static int mkv_write_stereo_mode(AVFormatContext *s, AVIOContext *pb,
>      return ret;
>  }
>  
> +static int mkv_write_video_projection(AVIOContext *pb, AVStream *st)
> +{
> +    AVSphericalMapping *spherical_mapping = (AVSphericalMapping*)av_stream_get_side_data(st, AV_PKT_DATA_SPHERICAL, NULL);
> +    ebml_master projection;
> +    int projection_type = 0;
> +
> +    AVIOContext *dyn_cp;
> +    uint8_t *projection_private_ptr = NULL;
> +    int ret, projection_private_size;
> +
> +    if (!spherical_mapping)
> +        return 0;
> +
> +    if (spherical_mapping->projection != AV_SPHERICAL_EQUIRECTANGULAR &&
> +        spherical_mapping->projection != AV_SPHERICAL_CUBEMAP) {
> +        av_log(pb, AV_LOG_WARNING, "Unsupported projection %d. projection not written.\n", spherical_mapping->projection);
> +        return 0;
> +    }
> +
> +    ret = avio_open_dyn_buf(&dyn_cp);
> +    if (ret < 0)
> +        return ret;
> +
> +    switch (spherical_mapping->projection) {
> +    case AV_SPHERICAL_EQUIRECTANGULAR:
> +        projection_type = 1;
> +
> +        /* Create ProjectionPrivate contents */
> +        avio_wb32(dyn_cp, 0); /* version = 0 & flags = 0 */
> +        avio_wb32(dyn_cp, 0); /* projection_bounds_top */
> +        avio_wb32(dyn_cp, 0); /* projection_bounds_bottom */
> +        avio_wb32(dyn_cp, 0); /* projection_bounds_left */
> +        avio_wb32(dyn_cp, 0); /* projection_bounds_right */

You could instead use a local 20 byte array, fill it using AV_WB32() and
then write it with put_ebml_binary().

Also, the latest change to the spec draft mentions ProjectionPrivate is
optional for Equirectangular projections if the contents are going to be
all zero.

> +        break;
> +    case AV_SPHERICAL_CUBEMAP:
> +        projection_type = 2;
> +
> +        /* Create ProjectionPrivate contents */
> +        avio_wb32(dyn_cp, 0); /* version = 0 & flags = 0 */
> +        avio_wb32(dyn_cp, 0); /* layout */
> +        avio_wb32(dyn_cp, 0); /* padding */
> +        break;
> +    }

Same, 12 byte array.

What if the user is trying to remux a matroska file that has a
ProjectionPrivate element or an mp4 with an equi box filled with non zero
values, for that matter?
I know you're the one behind the spec in question, but wouldn't it be a
better idea to wait until AVSphericalMapping gets a way to propagate this
kind of information before adding support for muxing video projection
elements? Or maybe try to implement it right now...

This also applies to the mp4 patch.

> +
> +    projection_private_size = avio_close_dyn_buf(dyn_cp, &projection_private_ptr);

The dynbuf should ideally contain the whole Projection master, so you can
then pass its size to start_ebml_master() instead of zero.
See mkv_write_video_color().

> +
> +    projection = start_ebml_master(pb, MATROSKA_ID_VIDEOPROJECTION, 0);
> +    put_ebml_uint(pb, MATROSKA_ID_VIDEOPROJECTIONTYPE, projection_type);
> +    if (projection_private_size)
> +        put_ebml_binary(pb, MATROSKA_ID_VIDEOPROJECTIONPRIVATE, projection_private_ptr, projection_private_size);
> +
> +    put_ebml_float(pb, MATROSKA_ID_VIDEOPROJECTIONPOSEYAW, (float)(spherical_mapping->yaw) / (1 << 16));
> +    put_ebml_float(pb, MATROSKA_ID_VIDEOPROJECTIONPOSEPITCH, (float)(spherical_mapping->pitch) / (1 << 16));
> +    put_ebml_float(pb, MATROSKA_ID_VIDEOPROJECTIONPOSEROLL, (float)(spherical_mapping->roll) / (1 << 16));
> +    end_ebml_master(pb, projection);
> +
> +    av_free(projection_private_ptr);
> +
> +    return 0;
> +}
> +
>  static int mkv_write_track(AVFormatContext *s, MatroskaMuxContext *mkv,
>                             int i, AVIOContext *pb, int default_stream_exists)
>  {
> @@ -1251,6 +1312,9 @@ static int mkv_write_track(AVFormatContext *s, MatroskaMuxContext *mkv,
>          ret = mkv_write_video_color(pb, par, st);
>          if (ret < 0)
>              return ret;
> +        ret = mkv_write_video_projection(pb, st);

This needs to be inside a check for strict experimental compliance
nonetheless.

> +        if (ret < 0)
> +            return ret;
>          end_ebml_master(pb, subinfo);
>          break;
>  
> -- 2.11.0.483.g087da7b7c-goog
> 
> 
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>

On Fri, Jan 27, 2017 at 5:46 PM James Almer <jamrial@gmail.com> wrote:

> On 1/27/2017 5:12 PM, Aaron Colwell wrote:
> > Adding support for writing spherical metadata in Matroska.
> >
> >
> > 0001-matroskaenc-Add-support-for-writing-video-projection.patch
> >
> >
> > From 5a9cf56bf3114186412bb5572b153f886edb6ddb Mon Sep 17 00:00:00 2001
> > From: Aaron Colwell <acolwell@google.com>
> > Date: Fri, 27 Jan 2017 12:07:25 -0800
> > Subject: [PATCH] matroskaenc: Add support for writing video projection
> >  element.
> >
> > Adding support for writing spherical metadata in Matroska.
> > ---
> >  libavformat/matroskaenc.c | 64
> +++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 64 insertions(+)
> >
> > diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
> > index f731b678b9..1b186db223 100644
> > --- a/libavformat/matroskaenc.c
> > +++ b/libavformat/matroskaenc.c
> > @@ -1038,6 +1038,67 @@ static int mkv_write_stereo_mode(AVFormatContext
> *s, AVIOContext *pb,
> >      return ret;
> >  }
> >
> > +static int mkv_write_video_projection(AVIOContext *pb, AVStream *st)
> > +{
> > +    AVSphericalMapping *spherical_mapping =
> (AVSphericalMapping*)av_stream_get_side_data(st, AV_PKT_DATA_SPHERICAL,
> NULL);
> > +    ebml_master projection;
> > +    int projection_type = 0;
> > +
> > +    AVIOContext *dyn_cp;
> > +    uint8_t *projection_private_ptr = NULL;
> > +    int ret, projection_private_size;
> > +
> > +    if (!spherical_mapping)
> > +        return 0;
> > +
> > +    if (spherical_mapping->projection != AV_SPHERICAL_EQUIRECTANGULAR &&
> > +        spherical_mapping->projection != AV_SPHERICAL_CUBEMAP) {
> > +        av_log(pb, AV_LOG_WARNING, "Unsupported projection %d.
> projection not written.\n", spherical_mapping->projection);
> > +        return 0;
> > +    }
> > +
> > +    ret = avio_open_dyn_buf(&dyn_cp);
> > +    if (ret < 0)
> > +        return ret;
> > +
> > +    switch (spherical_mapping->projection) {
> > +    case AV_SPHERICAL_EQUIRECTANGULAR:
> > +        projection_type = 1;
> > +
> > +        /* Create ProjectionPrivate contents */
> > +        avio_wb32(dyn_cp, 0); /* version = 0 & flags = 0 */
> > +        avio_wb32(dyn_cp, 0); /* projection_bounds_top */
> > +        avio_wb32(dyn_cp, 0); /* projection_bounds_bottom */
> > +        avio_wb32(dyn_cp, 0); /* projection_bounds_left */
> > +        avio_wb32(dyn_cp, 0); /* projection_bounds_right */
>
> You could instead use a local 20 byte array, fill it using AV_WB32() and
> then write it with put_ebml_binary().
>

I was mainly using this form since that is what the code would have to look
like once AVSphericalMapping actually contained this data.


>
> Also, the latest change to the spec draft mentions ProjectionPrivate is
> optional for Equirectangular projections if the contents are going to be
> all zero.
>

True. I could just drop this if that is preferred.


>
> > +        break;
> > +    case AV_SPHERICAL_CUBEMAP:
> > +        projection_type = 2;
> > +
> > +        /* Create ProjectionPrivate contents */
> > +        avio_wb32(dyn_cp, 0); /* version = 0 & flags = 0 */
> > +        avio_wb32(dyn_cp, 0); /* layout */
> > +        avio_wb32(dyn_cp, 0); /* padding */
> > +        break;
> > +    }
>
> Same, 12 byte array.
>
> What if the user is trying to remux a matroska file that has a
> ProjectionPrivate element or an mp4 with an equi box filled with non zero
> values, for that matter?
>

yeah. I'm a little confused why the demuxing code didn't implement this to
begin with.


> I know you're the one behind the spec in question, but wouldn't it be a
> better idea to wait until AVSphericalMapping gets a way to propagate this
> kind of information before adding support for muxing video projection
> elements? Or maybe try to implement it right now...
>

I'm happy to implement support for the projection specific info. What would
be the best way to proceed. It seems like I could just place a union with
projection specific structs in AVSphericalMapping. I'd also like some
advice around how to sequence the set of changes. My preference would be to
add the union & fields to AVSphericalMapping and update at least one
demuxer to at least justify the presence of the fields in a single patch.
Not sure if that is the preferred way to go about this though.


>
> This also applies to the mp4 patch.
>

Understood and makes sense.


>
> > +
> > +    projection_private_size = avio_close_dyn_buf(dyn_cp,
> &projection_private_ptr);
>
> The dynbuf should ideally contain the whole Projection master, so you can
> then pass its size to start_ebml_master() instead of zero.
> See mkv_write_video_color().
>
>
I'm a little confused about what you mean by passing the size to
start_ebml_master() it looks like both the calls I see in
mkv_write_video_color() pass in zero.


> > +
> > +    projection = start_ebml_master(pb, MATROSKA_ID_VIDEOPROJECTION, 0);
> > +    put_ebml_uint(pb, MATROSKA_ID_VIDEOPROJECTIONTYPE, projection_type);
> > +    if (projection_private_size)
> > +        put_ebml_binary(pb, MATROSKA_ID_VIDEOPROJECTIONPRIVATE,
> projection_private_ptr, projection_private_size);
> > +
> > +    put_ebml_float(pb, MATROSKA_ID_VIDEOPROJECTIONPOSEYAW,
> (float)(spherical_mapping->yaw) / (1 << 16));
> > +    put_ebml_float(pb, MATROSKA_ID_VIDEOPROJECTIONPOSEPITCH,
> (float)(spherical_mapping->pitch) / (1 << 16));
> > +    put_ebml_float(pb, MATROSKA_ID_VIDEOPROJECTIONPOSEROLL,
> (float)(spherical_mapping->roll) / (1 << 16));
> > +    end_ebml_master(pb, projection);
> > +
> > +    av_free(projection_private_ptr);
> > +
> > +    return 0;
> > +}
> > +
> >  static int mkv_write_track(AVFormatContext *s, MatroskaMuxContext *mkv,
> >                             int i, AVIOContext *pb, int
> default_stream_exists)
> >  {
> > @@ -1251,6 +1312,9 @@ static int mkv_write_track(AVFormatContext *s,
> MatroskaMuxContext *mkv,
> >          ret = mkv_write_video_color(pb, par, st);
> >          if (ret < 0)
> >              return ret;
> > +        ret = mkv_write_video_projection(pb, st);
>
> This needs to be inside a check for strict experimental compliance
> nonetheless.
>

Ok. I'm assuming you mean adding something like

if (s->strict_std_compliance > FF_COMPLIANCE_EXPERIMENTAL) {
    av_log(s, AV_LOG_ERROR,
                 "Spherical metadata in Matroska support is experimental,
add "
                 "'-strict %d' if you want to use it.\n",
                 FF_COMPLIANCE_EXPERIMENTAL);
    return AVERROR_EXPERIMENTAL;
}

Thanks for your help.

Aaron


>
> > +        if (ret < 0)
> > +            return ret;
> >          end_ebml_master(pb, subinfo);
> >          break;
> >
> > -- 2.11.0.483.g087da7b7c-goog
> >
> >
> >
> > _______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel@ffmpeg.org
> > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>

On 1/27/2017 11:21 PM, Aaron Colwell wrote:
> On Fri, Jan 27, 2017 at 5:46 PM James Almer <jamrial@gmail.com> wrote:
> 
>> On 1/27/2017 5:12 PM, Aaron Colwell wrote:
>>> Adding support for writing spherical metadata in Matroska.
>>>
>>>
>>> 0001-matroskaenc-Add-support-for-writing-video-projection.patch
>>>
>>>
>>> From 5a9cf56bf3114186412bb5572b153f886edb6ddb Mon Sep 17 00:00:00 2001
>>> From: Aaron Colwell <acolwell@google.com>
>>> Date: Fri, 27 Jan 2017 12:07:25 -0800
>>> Subject: [PATCH] matroskaenc: Add support for writing video projection
>>>  element.
>>>
>>> Adding support for writing spherical metadata in Matroska.
>>> ---
>>>  libavformat/matroskaenc.c | 64
>> +++++++++++++++++++++++++++++++++++++++++++++++
>>>  1 file changed, 64 insertions(+)
>>>
>>> diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
>>> index f731b678b9..1b186db223 100644
>>> --- a/libavformat/matroskaenc.c
>>> +++ b/libavformat/matroskaenc.c
>>> @@ -1038,6 +1038,67 @@ static int mkv_write_stereo_mode(AVFormatContext
>> *s, AVIOContext *pb,
>>>      return ret;
>>>  }
>>>
>>> +static int mkv_write_video_projection(AVIOContext *pb, AVStream *st)
>>> +{
>>> +    AVSphericalMapping *spherical_mapping =
>> (AVSphericalMapping*)av_stream_get_side_data(st, AV_PKT_DATA_SPHERICAL,
>> NULL);
>>> +    ebml_master projection;
>>> +    int projection_type = 0;
>>> +
>>> +    AVIOContext *dyn_cp;
>>> +    uint8_t *projection_private_ptr = NULL;
>>> +    int ret, projection_private_size;
>>> +
>>> +    if (!spherical_mapping)
>>> +        return 0;
>>> +
>>> +    if (spherical_mapping->projection != AV_SPHERICAL_EQUIRECTANGULAR &&
>>> +        spherical_mapping->projection != AV_SPHERICAL_CUBEMAP) {
>>> +        av_log(pb, AV_LOG_WARNING, "Unsupported projection %d.
>> projection not written.\n", spherical_mapping->projection);
>>> +        return 0;
>>> +    }
>>> +
>>> +    ret = avio_open_dyn_buf(&dyn_cp);
>>> +    if (ret < 0)
>>> +        return ret;
>>> +
>>> +    switch (spherical_mapping->projection) {
>>> +    case AV_SPHERICAL_EQUIRECTANGULAR:
>>> +        projection_type = 1;
>>> +
>>> +        /* Create ProjectionPrivate contents */
>>> +        avio_wb32(dyn_cp, 0); /* version = 0 & flags = 0 */
>>> +        avio_wb32(dyn_cp, 0); /* projection_bounds_top */
>>> +        avio_wb32(dyn_cp, 0); /* projection_bounds_bottom */
>>> +        avio_wb32(dyn_cp, 0); /* projection_bounds_left */
>>> +        avio_wb32(dyn_cp, 0); /* projection_bounds_right */
>>
>> You could instead use a local 20 byte array, fill it using AV_WB32() and
>> then write it with put_ebml_binary().
>>
> 
> I was mainly using this form since that is what the code would have to look
> like once AVSphericalMapping actually contained this data.

I know. I meant doing something like

    uint8_t private[20];

    AV_WB32(private + 0, 0);
    AV_WB32(private + 4, projection_bounds_top);
    AV_WB32(private + 8, projection_bounds_bottom);
    AV_WB32(private + 12, projection_bounds_left);
    AV_WB32(private + 16, projection_bounds_right);

    put_ebml_binary(dyn_cp, MATROSKA_ID_VIDEOPROJECTIONPRIVATE, private, sizeof(private));

Then the same for Cubemap, but it's mostly a nit.

> 
> 
>>
>> Also, the latest change to the spec draft mentions ProjectionPrivate is
>> optional for Equirectangular projections if the contents are going to be
>> all zero.
>>
> 
> True. I could just drop this if that is preferred.
> 
> 
>>
>>> +        break;
>>> +    case AV_SPHERICAL_CUBEMAP:
>>> +        projection_type = 2;
>>> +
>>> +        /* Create ProjectionPrivate contents */
>>> +        avio_wb32(dyn_cp, 0); /* version = 0 & flags = 0 */
>>> +        avio_wb32(dyn_cp, 0); /* layout */
>>> +        avio_wb32(dyn_cp, 0); /* padding */
>>> +        break;
>>> +    }
>>
>> Same, 12 byte array.
>>
>> What if the user is trying to remux a matroska file that has a
>> ProjectionPrivate element or an mp4 with an equi box filled with non zero
>> values, for that matter?
>>
> 
> yeah. I'm a little confused why the demuxing code didn't implement this to
> begin with.

Vittorio (The one that implemented AVSphericalMapping) tried to add this at
first, but then removed it because he wasn't sure if he was doing it right.

> 
> 
>> I know you're the one behind the spec in question, but wouldn't it be a
>> better idea to wait until AVSphericalMapping gets a way to propagate this
>> kind of information before adding support for muxing video projection
>> elements? Or maybe try to implement it right now...
>>
> 
> I'm happy to implement support for the projection specific info. What would
> be the best way to proceed. It seems like I could just place a union with
> projection specific structs in AVSphericalMapping. I'd also like some

I'm CCing Vittorio so he can chim in. I personally have no real preference.

> advice around how to sequence the set of changes. My preference would be to
> add the union & fields to AVSphericalMapping and update at least one
> demuxer to at least justify the presence of the fields in a single patch.
> Not sure if that is the preferred way to go about this though.

Yeah, one patch to extend AVSphericalMapping, then separate patches to make
use of the new fields in both the mov and matroska demuxers.

> 
> 
>>
>> This also applies to the mp4 patch.
>>
> 
> Understood and makes sense.
> 
> 
>>
>>> +
>>> +    projection_private_size = avio_close_dyn_buf(dyn_cp,
>> &projection_private_ptr);
>>
>> The dynbuf should ideally contain the whole Projection master, so you can
>> then pass its size to start_ebml_master() instead of zero.
>> See mkv_write_video_color().
>>
>>
> I'm a little confused about what you mean by passing the size to
> start_ebml_master() it looks like both the calls I see in
> mkv_write_video_color() pass in zero.

Ah, now that's an oversight. start_ebml_master() there was meant to use
the return value from avio_close_dyn_buf(). I'll fix that later.

The idea is to use a dynamic buffer to write every child element from the
master (In this case Colour or Projection), then dump its contents into the
output AVIOContext. By knowing the entire master's size you can save up to
seven bytes when writing it.

> 
> 
>>> +
>>> +    projection = start_ebml_master(pb, MATROSKA_ID_VIDEOPROJECTION, 0);
>>> +    put_ebml_uint(pb, MATROSKA_ID_VIDEOPROJECTIONTYPE, projection_type);
>>> +    if (projection_private_size)
>>> +        put_ebml_binary(pb, MATROSKA_ID_VIDEOPROJECTIONPRIVATE,
>> projection_private_ptr, projection_private_size);
>>> +
>>> +    put_ebml_float(pb, MATROSKA_ID_VIDEOPROJECTIONPOSEYAW,
>> (float)(spherical_mapping->yaw) / (1 << 16));
>>> +    put_ebml_float(pb, MATROSKA_ID_VIDEOPROJECTIONPOSEPITCH,
>> (float)(spherical_mapping->pitch) / (1 << 16));
>>> +    put_ebml_float(pb, MATROSKA_ID_VIDEOPROJECTIONPOSEROLL,
>> (float)(spherical_mapping->roll) / (1 << 16));
>>> +    end_ebml_master(pb, projection);

Missed it the first time, but these should be cast to double, not float.

>>> +
>>> +    av_free(projection_private_ptr);
>>> +
>>> +    return 0;
>>> +}
>>> +
>>>  static int mkv_write_track(AVFormatContext *s, MatroskaMuxContext *mkv,
>>>                             int i, AVIOContext *pb, int
>> default_stream_exists)
>>>  {
>>> @@ -1251,6 +1312,9 @@ static int mkv_write_track(AVFormatContext *s,
>> MatroskaMuxContext *mkv,
>>>          ret = mkv_write_video_color(pb, par, st);
>>>          if (ret < 0)
>>>              return ret;
>>> +        ret = mkv_write_video_projection(pb, st);
>>
>> This needs to be inside a check for strict experimental compliance
>> nonetheless.
>>
> 
> Ok. I'm assuming you mean adding something like
> 
> if (s->strict_std_compliance > FF_COMPLIANCE_EXPERIMENTAL) {
>     av_log(s, AV_LOG_ERROR,
>                  "Spherical metadata in Matroska support is experimental,
> add "
>                  "'-strict %d' if you want to use it.\n",
>                  FF_COMPLIANCE_EXPERIMENTAL);
>     return AVERROR_EXPERIMENTAL;
> }

Yes, but preferably make it

if (s->strict_std_compliance <= FF_COMPLIANCE_EXPERIMENTAL) {
    ret = mkv_write_video_projection(pb, st);
    if (ret < 0)
        return ret;
}

> 
> Thanks for your help.
> 
> Aaron
> 
> 
>>
>>> +        if (ret < 0)
>>> +            return ret;
>>>          end_ebml_master(pb, subinfo);
>>>          break;
>>>
>>> -- 2.11.0.483.g087da7b7c-goog
>>>
>>>
>>>
>>> _______________________________________________
>>> ffmpeg-devel mailing list
>>> ffmpeg-devel@ffmpeg.org
>>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>>
>>
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>

On Sat, Jan 28, 2017 at 4:13 AM, James Almer <jamrial@gmail.com> wrote:
> On 1/27/2017 11:21 PM, Aaron Colwell wrote:
>> On Fri, Jan 27, 2017 at 5:46 PM James Almer <jamrial@gmail.com> wrote:
>>
>> yeah. I'm a little confused why the demuxing code didn't implement this to
>> begin with.
>
> Vittorio (The one that implemented AVSphericalMapping) tried to add this at
> first, but then removed it because he wasn't sure if he was doing it right.

Hi,
yes this was included initially but then we found out what those
fields were really for, and I didn't want to make the users get as
confused as we were. As a matter of fact Aaron I mentioned this to you
when I proposed that we probably should have separated the classic
equi projection from the tiled one in the specification, in order to
simplify the implementation.

>>> I know you're the one behind the spec in question, but wouldn't it be a
>>> better idea to wait until AVSphericalMapping gets a way to propagate this
>>> kind of information before adding support for muxing video projection
>>> elements? Or maybe try to implement it right now...
>>>
>>
>> I'm happy to implement support for the projection specific info. What would
>> be the best way to proceed. It seems like I could just place a union with
>> projection specific structs in AVSphericalMapping. I'd also like some
>
> I'm CCing Vittorio so he can chim in. I personally have no real preference.

The best way in my opinion is to add a third type, such as
AV_SPHERICAL_TILED_EQUI, and add the relevant fields in
AVSphericalMapping, with a clear description about the actual use case
for them, mentioning that they are used only in format. By the way,
why do you mention adding a union? I think four plain fields should
do.

Please keep me in CC.

On Tue, Jan 31, 2017 at 2:12 AM Vittorio Giovara <vittorio.giovara@gmail.com>
wrote:

> On Sat, Jan 28, 2017 at 4:13 AM, James Almer <jamrial@gmail.com> wrote:
> > On 1/27/2017 11:21 PM, Aaron Colwell wrote:
> >> On Fri, Jan 27, 2017 at 5:46 PM James Almer <jamrial@gmail.com> wrote:
> >>
> >> yeah. I'm a little confused why the demuxing code didn't implement this
> to
> >> begin with.
> >
> > Vittorio (The one that implemented AVSphericalMapping) tried to add this
> at
> > first, but then removed it because he wasn't sure if he was doing it
> right.
>
> Hi,
> yes this was included initially but then we found out what those
> fields were really for, and I didn't want to make the users get as
> confused as we were. As a matter of fact Aaron I mentioned this to you
> when I proposed that we probably should have separated the classic
> equi projection from the tiled one in the specification, in order to
> simplify the implementation.
>

Like I said before, it is not a different projection. It is still
equirectangular and those parameters just crops the projection. It is very
simple to just verify that the fields are zero if you don't want to support
the cropped version.


>
> >>> I know you're the one behind the spec in question, but wouldn't it be a
> >>> better idea to wait until AVSphericalMapping gets a way to propagate
> this
> >>> kind of information before adding support for muxing video projection
> >>> elements? Or maybe try to implement it right now...
> >>>
> >>
> >> I'm happy to implement support for the projection specific info. What
> would
> >> be the best way to proceed. It seems like I could just place a union
> with
> >> projection specific structs in AVSphericalMapping. I'd also like some
> >
> > I'm CCing Vittorio so he can chim in. I personally have no real
> preference.
>
> The best way in my opinion is to add a third type, such as
> AV_SPHERICAL_TILED_EQUI, and add the relevant fields in
> AVSphericalMapping, with a clear description about the actual use case
> for them, mentioning that they are used only in format. By the way,
> why do you mention adding a union? I think four plain fields should
> do.
>

I don't think it is worth having the extra enum value for this. All the
cropped fields do is control how you generate the spherical mesh or control
the shader used to render the projection. If players don't want to support
it they can just check to see that all the fields are zero and error out if
not.

I was suggesting using a union because the projection bounds fields are for
equirect, and there are different fields for the cubemap & mesh
projections. I figured that the enum + union of structs might be a
reasonable way to organize the projection specific fields.



>
> Please keep me in CC.
>

Will do.

Aaron


> --
> Vittorio
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>

On 1/31/2017 12:40 PM, Aaron Colwell wrote:
> 
> 
> On Tue, Jan 31, 2017 at 2:12 AM Vittorio Giovara <vittorio.giovara@gmail.com <mailto:vittorio.giovara@gmail.com>> wrote:
> 
> On Sat, Jan 28, 2017 at 4:13 AM, James Almer <jamrial@gmail.com <mailto:jamrial@gmail.com>> wrote:
>> On 1/27/2017 11:21 PM, Aaron Colwell wrote:
>>> On Fri, Jan 27, 2017 at 5:46 PM James Almer <jamrial@gmail.com <mailto:jamrial@gmail.com>> wrote:
>>>
>>> yeah. I'm a little confused why the demuxing code didn't implement this to
>>> begin with.
>>
>> Vittorio (The one that implemented AVSphericalMapping) tried to add this at
>> first, but then removed it because he wasn't sure if he was doing it right.
> 
> Hi,
> yes this was included initially but then we found out what those
> fields were really for, and I didn't want to make the users get as
> confused as we were. As a matter of fact Aaron I mentioned this to you
> when I proposed that we probably should have separated the classic
> equi projection from the tiled one in the specification, in order to
> simplify the implementation.
> 
> 
> Like I said before, it is not a different projection. It is still equirectangular and those parameters just crops the projection. It is very simple to just verify that the fields are zero if you don't want to support the cropped version.
>  
> 
> 
>>>> I know you're the one behind the spec in question, but wouldn't it be a
>>>> better idea to wait until AVSphericalMapping gets a way to propagate this
>>>> kind of information before adding support for muxing video projection
>>>> elements? Or maybe try to implement it right now...
>>>>
>>>
>>> I'm happy to implement support for the projection specific info. What would
>>> be the best way to proceed. It seems like I could just place a union with
>>> projection specific structs in AVSphericalMapping. I'd also like some
>>
>> I'm CCing Vittorio so he can chim in. I personally have no real preference.
> 
> The best way in my opinion is to add a third type, such as
> AV_SPHERICAL_TILED_EQUI, and add the relevant fields in
> AVSphericalMapping, with a clear description about the actual use case
> for them, mentioning that they are used only in format. By the way,
> why do you mention adding a union? I think four plain fields should
> do.
> 
> 
> I don't think it is worth having the extra enum value for this. All the cropped fields do is control how you generate the spherical mesh or control the shader used to render the projection. If players don't want to support it they can just check to see that all the fields are zero and error out if not. 
> 
> I was suggesting using a union because the projection bounds fields are for equirect, and there are different fields for the cubemap & mesh projections. I figured that the enum + union of structs might be a reasonable way to organize the projection specific fields.
> 
>  
> 
> 
> Please keep me in CC.
> 
> 
> Will do.
> 
> Aaron

You didn't CC him, so doing it now in case you also didn't BCC him.

On Thu, Feb 2, 2017 at 9:34 PM, James Almer <jamrial@gmail.com> wrote:
> On 1/31/2017 12:40 PM, Aaron Colwell wrote:
>>
>>
>> On Tue, Jan 31, 2017 at 2:12 AM Vittorio Giovara <vittorio.giovara@gmail.com <mailto:vittorio.giovara@gmail.com>> wrote:
>>
>> On Sat, Jan 28, 2017 at 4:13 AM, James Almer <jamrial@gmail.com <mailto:jamrial@gmail.com>> wrote:
>>> On 1/27/2017 11:21 PM, Aaron Colwell wrote:
>>>> On Fri, Jan 27, 2017 at 5:46 PM James Almer <jamrial@gmail.com <mailto:jamrial@gmail.com>> wrote:
>>>>
>>>> yeah. I'm a little confused why the demuxing code didn't implement this to
>>>> begin with.
>>>
>>> Vittorio (The one that implemented AVSphericalMapping) tried to add this at
>>> first, but then removed it because he wasn't sure if he was doing it right.
>>
>> Hi,
>> yes this was included initially but then we found out what those
>> fields were really for, and I didn't want to make the users get as
>> confused as we were. As a matter of fact Aaron I mentioned this to you
>> when I proposed that we probably should have separated the classic
>> equi projection from the tiled one in the specification, in order to
>> simplify the implementation.
>>
>>
>> Like I said before, it is not a different projection. It is still equirectangular and those parameters just crops the projection. It is very simple to just verify that the fields are zero if you don't want to support the cropped version.

Hello,
I'm sorry but I heavily disagree. The tiled equirectangular projection
is something that cannot be used standalone, you have to do additional
mathematics and take into account different files or streams to
generate a "normal" or full-frame equirectangular projection. Having a
separate type allows to include extensions such as the bounds fields,
which can be safely ignored by the every user that do not need a tiled
projection.

It is too late to change the spec, but I do believe that the usecase
is different enough to add a new type, in order to not overcomplicate
the implementation.

>>>>> I know you're the one behind the spec in question, but wouldn't it be a
>>>>> better idea to wait until AVSphericalMapping gets a way to propagate this
>>>>> kind of information before adding support for muxing video projection
>>>>> elements? Or maybe try to implement it right now...
>>>>>
>>>>
>>>> I'm happy to implement support for the projection specific info. What would
>>>> be the best way to proceed. It seems like I could just place a union with
>>>> projection specific structs in AVSphericalMapping. I'd also like some
>>>
>>> I'm CCing Vittorio so he can chim in. I personally have no real preference.
>>
>> The best way in my opinion is to add a third type, such as
>> AV_SPHERICAL_TILED_EQUI, and add the relevant fields in
>> AVSphericalMapping, with a clear description about the actual use case
>> for them, mentioning that they are used only in format. By the way,
>> why do you mention adding a union? I think four plain fields should
>> do.
>>
>>
>> I don't think it is worth having the extra enum value for this. All the cropped fields do is control how you generate the spherical mesh or control the shader used to render the projection. If players don't want to support it they can just check to see that all the fields are zero and error out if not.

Why would you have them check these fields every time, when this can
be implicitly determined by the type semantics. I'm pretty sure API
users prefer this scenario

* check projection type
-> if normal_equi -> project it
-> if tiled_equi -> read additional data -> project it

rather than

* check projection type
-> if equi -> read additional data -> check if data needs additional
processing -> project it, or perform more operations before projecting

>> I was suggesting using a union because the projection bounds fields are for equirect, and there are different fields for the cubemap & mesh projections. I figured that the enum + union of structs might be a reasonable way to organize the projection specific fields.

This is a structure whose size does not depend on ABI and can be
extended as we like, there is no need to separate new fields in such a
way as long as they are properly documented, in my opinion.

Please keep me in CC.

On Fri, Feb 3, 2017 at 3:28 AM Vittorio Giovara <vittorio.giovara@gmail.com>
wrote:

> On Thu, Feb 2, 2017 at 9:34 PM, James Almer <jamrial@gmail.com> wrote:
> > On 1/31/2017 12:40 PM, Aaron Colwell wrote:
> >>
> >>
> >> On Tue, Jan 31, 2017 at 2:12 AM Vittorio Giovara <
> vittorio.giovara@gmail.com <mailto:vittorio.giovara@gmail.com>> wrote:
> >>
> >> On Sat, Jan 28, 2017 at 4:13 AM, James Almer <jamrial@gmail.com
> <mailto:jamrial@gmail.com>> wrote:
> >>> On 1/27/2017 11:21 PM, Aaron Colwell wrote:
> >>>> On Fri, Jan 27, 2017 at 5:46 PM James Almer <jamrial@gmail.com
> <mailto:jamrial@gmail.com>> wrote:
> >>>>
> >>>> yeah. I'm a little confused why the demuxing code didn't implement
> this to
> >>>> begin with.
> >>>
> >>> Vittorio (The one that implemented AVSphericalMapping) tried to add
> this at
> >>> first, but then removed it because he wasn't sure if he was doing it
> right.
> >>
> >> Hi,
> >> yes this was included initially but then we found out what those
> >> fields were really for, and I didn't want to make the users get as
> >> confused as we were. As a matter of fact Aaron I mentioned this to you
> >> when I proposed that we probably should have separated the classic
> >> equi projection from the tiled one in the specification, in order to
> >> simplify the implementation.
> >>
> >>
> >> Like I said before, it is not a different projection. It is still
> equirectangular and those parameters just crops the projection. It is very
> simple to just verify that the fields are zero if you don't want to support
> the cropped version.
>
> Hello,
> I'm sorry but I heavily disagree. The tiled equirectangular projection
> is something that cannot be used standalone, you have to do additional
> mathematics and take into account different files or streams to
> generate a "normal" or full-frame equirectangular projection. Having a
> separate type allows to include extensions such as the bounds fields,
> which can be safely ignored by the every user that do not need a tiled
> projection.
>

I still think you don't understand what these fields do given what you say
here. Yes there is a little more math. At the end of the day all these
fields do is specify a the min & max for the latitude & longitude. This
essentially translates to adding scale factors and offsets in your shader
or something similar in your 3D geometry creation logic. I get it if
implementations don't want to do this small bit of extra work, but saying
this is a different type seems strange because you wouldn't do this when
talking about cropped 2D images.


>
> It is too late to change the spec, but I do believe that the usecase
> is different enough to add a new type, in order to not overcomplicate
> the implementation.
>

It feels like you are just moving the problem to the demuxers and muxers
here. Adding a new type means all demuxers will have to contain logic to
generate these different types and all muxers will have to contain logic to
collapse these types back down to a single value.

I don't really want to keep arguing about this. If folks really want
different types then I'll do it just because I want to get reading and
writing of metadata working end-to-end. I'd like to make a small request to
use the term "cropped equirectagular" instead of "tiled equirectangular"
but I don't feel to strongly about that.


>
> >>>>> I know you're the one behind the spec in question, but wouldn't it
> be a
> >>>>> better idea to wait until AVSphericalMapping gets a way to propagate
> this
> >>>>> kind of information before adding support for muxing video projection
> >>>>> elements? Or maybe try to implement it right now...
> >>>>>
> >>>>
> >>>> I'm happy to implement support for the projection specific info. What
> would
> >>>> be the best way to proceed. It seems like I could just place a union
> with
> >>>> projection specific structs in AVSphericalMapping. I'd also like some
> >>>
> >>> I'm CCing Vittorio so he can chim in. I personally have no real
> preference.
> >>
> >> The best way in my opinion is to add a third type, such as
> >> AV_SPHERICAL_TILED_EQUI, and add the relevant fields in
> >> AVSphericalMapping, with a clear description about the actual use case
> >> for them, mentioning that they are used only in format. By the way,
> >> why do you mention adding a union? I think four plain fields should
> >> do.
> >>
> >>
> >> I don't think it is worth having the extra enum value for this. All the
> cropped fields do is control how you generate the spherical mesh or control
> the shader used to render the projection. If players don't want to support
> it they can just check to see that all the fields are zero and error out if
> not.
>
> Why would you have them check these fields every time, when this can
> be implicitly determined by the type semantics. I'm pretty sure API
> users prefer this scenario
>
> * check projection type
> -> if normal_equi -> project it
> -> if tiled_equi -> read additional data -> project it
>
> rather than
>
> * check projection type
> -> if equi -> read additional data -> check if data needs additional
> processing -> project it, or perform more operations before projecting
>

I think this mainly boils down to which code you want to burden with the
checks. My proposal puts the burden on the rendering code. Your proposal
puts it on every muxer and demuxer that deals with this information.


>
> >> I was suggesting using a union because the projection bounds fields are
> for equirect, and there are different fields for the cubemap & mesh
> projections. I figured that the enum + union of structs might be a
> reasonable way to organize the projection specific fields.
>
> This is a structure whose size does not depend on ABI and can be
> extended as we like, there is no need to separate new fields in such a
> way as long as they are properly documented, in my opinion.
>

I suppose this is just a difference in style so I don't feel too strongly
about it. I was just trying to use unions & structs here to make it a
little clearer about which fields are expected to be valid and when. The
fields seemed to be disjoint sets so I was trying to use language features
to convey that.

I'd also like to float a potential alternative solution. We could just
convey the projection private data as a size and byte array in this struct.
That would allow me to pass the metadata from demuxers to muxers so I can
achieve my goals, and allow actual parsing of the information to be
deferred until someone needs it. How do you feel about this compromise
position?


>
> Please keep me in CC.
>

Done for real this time. :) Sorry about before. Reply all didn't do what I
expected it to do.

Aaron

On Fri, Feb 3, 2017 at 5:57 PM, Aaron Colwell <acolwell@google.com> wrote:
> I still think you don't understand what these fields do given what you say
> here. Yes there is a little more math. At the end of the day all these
> fields do is specify a the min & max for the latitude & longitude. This
> essentially translates to adding scale factors and offsets in your shader or
> something similar in your 3D geometry creation logic. I get it if
> implementations don't want to do this small bit of extra work, but saying
> this is a different type seems strange because you wouldn't do this when
> talking about cropped 2D images.

If I don't understand these fields, maybe the specification could do a
better job at explaining what they are for ;)

I am aware that the final projection is the same, but the actual
problem is that if we don't introduce a new type we would be conveying
a different semantics to a single projection type. In other words we
require applications to behave differently according to two different
fields (the projection type and the offset fields) rather than a
single one. So yes, the projection is the same, the usecase is
different, the app behaviour is different, the final processing is
different -- I think that all this warrants a separate type.

If I still didn't get my point across, let's try with an example: an
application that does not support the tiled equirectangular
projection, with a separate type it can immediately discern that it is
an unsupported type and inform the user, with a single type, instead,
it has to sort-of-implement the tiling and check for fields that
should not matter. Another example: look at AVStereo3DType, there is a
side-by-side and a side-by-side-quincunx : an application that does
not support quincux can just abort and notify the user, if they were
two separate fields, you could have applications that do not support
quincunx but try to render the side-by-side part (which usually
results in a garbage rendering).

So I reiterate that in my opinion a separate enum value which
"notifies" apps that they should check additional types is the way to
go.

>> It is too late to change the spec, but I do believe that the usecase
>> is different enough to add a new type, in order to not overcomplicate
>> the implementation.
>
>
> It feels like you are just moving the problem to the demuxers and muxers
> here. Adding a new type means all demuxers will have to contain logic to
> generate these different types and all muxers will have to contain logic to
> collapse these types back down to a single value.

Yes, I would a 100% add more logic to the 2 muxers and 2 demuxers that
implement this spec rather than the thousands applications that
implement the ffmpeg api. Also the "different logic" is literally an
"if" case, if not little else.

> I don't really want to keep arguing about this. If folks really want
> different types then I'll do it just because I want to get reading and
> writing of metadata working end-to-end. I'd like to make a small request to
> use the term "cropped equirectagular" instead of "tiled equirectangular" but
> I don't feel to strongly about that.

Please don't, "cropped" has entirely different meaning, and it's
already confusing that you can do bitstream level cropping and surface
cropping. If you really hate the term "tiled" you could use "bounded
equirectangular" as it is used in the spec.

> I suppose this is just a difference in style so I don't feel too strongly
> about it. I was just trying to use unions & structs here to make it a little
> clearer about which fields are expected to be valid and when. The fields
> seemed to be disjoint sets so I was trying to use language features to
> convey that.
>
> I'd also like to float a potential alternative solution. We could just
> convey the projection private data as a size and byte array in this struct.
> That would allow me to pass the metadata from demuxers to muxers so I can
> achieve my goals, and allow actual parsing of the information to be deferred
> until someone needs it. How do you feel about this compromise position?

Again I don't see any advantage in using your proposal, it makes the
code difficult to read and hard to debug. Binary metadata are
intrinsically bad in my opinion, for this use case you could just add
four fields, and read/write four times.

I still have the code I had around when I implemented that.

+        projection = AV_SPHERICAL_EQUIRECTANGULAR;
+
+        /* 0.32 fixed point */
+        t = sc->height * (float) avio_rb32(pb) / ((uint64_t) 1 << 32);
+        b = sc->height * (float) avio_rb32(pb) / ((uint64_t) 1 << 32);
+        l = sc->width * (float) avio_rb32(pb) / ((uint64_t) 1 << 32);
+        r = sc->width * (float) avio_rb32(pb) / ((uint64_t) 1 << 32);

[...]

+    sc->spherical->left_offset   = l;
+    sc->spherical->top_offset    = t;
+    sc->spherical->right_offset  = r;
+    sc->spherical->bottom_offset = b;

(and the reverse for writing of course).

Given this insistence on using a separate type, the fact that the "tiled
equirect" is under specified, and folks don't actually care about the
"tiled equirect" case right now could I just add code to mux the 2 cases
that are already well specified? I could also send out a patch to fix the
demuxers so they reject the "tiled equirect" cases for now. This seems like
reasonable compromise to allow end-to-end preservation of non-controversial
use cases. That is really all I'm trying to do right now.

Aaron

On Sat, Feb 4, 2017 at 7:46 AM Vittorio Giovara <vittorio.giovara@gmail.com>
wrote:

> On Fri, Feb 3, 2017 at 5:57 PM, Aaron Colwell <acolwell@google.com> wrote:
> > I still think you don't understand what these fields do given what you
> say
> > here. Yes there is a little more math. At the end of the day all these
> > fields do is specify a the min & max for the latitude & longitude. This
> > essentially translates to adding scale factors and offsets in your
> shader or
> > something similar in your 3D geometry creation logic. I get it if
> > implementations don't want to do this small bit of extra work, but saying
> > this is a different type seems strange because you wouldn't do this when
> > talking about cropped 2D images.
>
> If I don't understand these fields, maybe the specification could do a
> better job at explaining what they are for ;)
>
> I am aware that the final projection is the same, but the actual
> problem is that if we don't introduce a new type we would be conveying
> a different semantics to a single projection type. In other words we
> require applications to behave differently according to two different
> fields (the projection type and the offset fields) rather than a
> single one. So yes, the projection is the same, the usecase is
> different, the app behaviour is different, the final processing is
> different -- I think that all this warrants a separate type.
>
> If I still didn't get my point across, let's try with an example: an
> application that does not support the tiled equirectangular
> projection, with a separate type it can immediately discern that it is
> an unsupported type and inform the user, with a single type, instead,
> it has to sort-of-implement the tiling and check for fields that
> should not matter. Another example: look at AVStereo3DType, there is a
> side-by-side and a side-by-side-quincunx : an application that does
> not support quincux can just abort and notify the user, if they were
> two separate fields, you could have applications that do not support
> quincunx but try to render the side-by-side part (which usually
> results in a garbage rendering).
>
> So I reiterate that in my opinion a separate enum value which
> "notifies" apps that they should check additional types is the way to
> go.
>
> >> It is too late to change the spec, but I do believe that the usecase
> >> is different enough to add a new type, in order to not overcomplicate
> >> the implementation.
> >
> >
> > It feels like you are just moving the problem to the demuxers and muxers
> > here. Adding a new type means all demuxers will have to contain logic to
> > generate these different types and all muxers will have to contain logic
> to
> > collapse these types back down to a single value.
>
> Yes, I would a 100% add more logic to the 2 muxers and 2 demuxers that
> implement this spec rather than the thousands applications that
> implement the ffmpeg api. Also the "different logic" is literally an
> "if" case, if not little else.
>
> > I don't really want to keep arguing about this. If folks really want
> > different types then I'll do it just because I want to get reading and
> > writing of metadata working end-to-end. I'd like to make a small request
> to
> > use the term "cropped equirectagular" instead of "tiled equirectangular"
> but
> > I don't feel to strongly about that.
>
> Please don't, "cropped" has entirely different meaning, and it's
> already confusing that you can do bitstream level cropping and surface
> cropping. If you really hate the term "tiled" you could use "bounded
> equirectangular" as it is used in the spec.
>
> > I suppose this is just a difference in style so I don't feel too strongly
> > about it. I was just trying to use unions & structs here to make it a
> little
> > clearer about which fields are expected to be valid and when. The fields
> > seemed to be disjoint sets so I was trying to use language features to
> > convey that.
> >
> > I'd also like to float a potential alternative solution. We could just
> > convey the projection private data as a size and byte array in this
> struct.
> > That would allow me to pass the metadata from demuxers to muxers so I can
> > achieve my goals, and allow actual parsing of the information to be
> deferred
> > until someone needs it. How do you feel about this compromise position?
>
> Again I don't see any advantage in using your proposal, it makes the
> code difficult to read and hard to debug. Binary metadata are
> intrinsically bad in my opinion, for this use case you could just add
> four fields, and read/write four times.
>
> I still have the code I had around when I implemented that.
>
> +        projection = AV_SPHERICAL_EQUIRECTANGULAR;
> +
> +        /* 0.32 fixed point */
> +        t = sc->height * (float) avio_rb32(pb) / ((uint64_t) 1 << 32);
> +        b = sc->height * (float) avio_rb32(pb) / ((uint64_t) 1 << 32);
> +        l = sc->width * (float) avio_rb32(pb) / ((uint64_t) 1 << 32);
> +        r = sc->width * (float) avio_rb32(pb) / ((uint64_t) 1 << 32);
>
> [...]
>
> +    sc->spherical->left_offset   = l;
> +    sc->spherical->top_offset    = t;
> +    sc->spherical->right_offset  = r;
> +    sc->spherical->bottom_offset = b;
>
> (and the reverse for writing of course).
> --
> Vittorio
>