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

[Aaron Colwell]

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 
wrote:

> On Fri, Feb 3, 2017 at 5:57 PM, Aaron Colwell  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
> 

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

[Vittorio Giovara]

On Fri, Feb 3, 2017 at 5:57 PM, Aaron Colwell  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).
-- 

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

[Aaron Colwell]

On Fri, Feb 3, 2017 at 3:28 AM Vittorio Giovara 
wrote:

> On Thu, Feb 2, 2017 at 9:34 PM, James Almer  wrote:
> > On 1/31/2017 12:40 PM, Aaron Colwell wrote:
> >>
> >>
> >> On Tue, Jan 31, 2017 at 2:12 AM Vittorio Giovara <
> vittorio.giov...@gmail.com > wrote:
> >>
> >> On Sat, Jan 28, 2017 at 4:13 AM, James Almer  > wrote:
> >>> On 1/27/2017 11:21 PM, Aaron Colwell wrote:
>  On Fri, Jan 27, 2017 at 5:46 PM James Almer  > 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
>
> 

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

[Vittorio Giovara]

On Thu, Feb 2, 2017 at 9:34 PM, James Almer  wrote:
> On 1/31/2017 12:40 PM, Aaron Colwell wrote:
>>
>>
>> On Tue, Jan 31, 2017 at 2:12 AM Vittorio Giovara > > wrote:
>>
>> On Sat, Jan 28, 2017 at 4:13 AM, James Almer > > wrote:
>>> On 1/27/2017 11:21 PM, Aaron Colwell wrote:
 On Fri, Jan 27, 2017 at 5:46 PM James Almer > 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.
-- 
Vittorio
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[James Almer]

On 1/31/2017 12:40 PM, Aaron Colwell wrote:
> 
> 
> On Tue, Jan 31, 2017 at 2:12 AM Vittorio Giovara  > wrote:
> 
> On Sat, Jan 28, 2017 at 4:13 AM, James Almer  > wrote:
>> On 1/27/2017 11:21 PM, Aaron Colwell wrote:
>>> On Fri, Jan 27, 2017 at 5:46 PM James Almer >> > 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.

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Aaron Colwell]

On Tue, Jan 31, 2017 at 2:12 AM Vittorio Giovara 
wrote:

> On Sat, Jan 28, 2017 at 4:13 AM, James Almer  wrote:
> > On 1/27/2017 11:21 PM, Aaron Colwell wrote:
> >> On Fri, Jan 27, 2017 at 5:46 PM James Almer  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
>
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Vittorio Giovara]

On Sat, Jan 28, 2017 at 4:13 AM, James Almer  wrote:
> On 1/27/2017 11:21 PM, Aaron Colwell wrote:
>> On Fri, Jan 27, 2017 at 5:46 PM James Almer  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.
-- 
Vittorio
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[James Almer]

On 1/27/2017 11:21 PM, Aaron Colwell wrote:
> On Fri, Jan 27, 2017 at 5:46 PM James Almer  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 
>>> 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(_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 

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

[Aaron Colwell]

On Fri, Jan 27, 2017 at 5:46 PM James Almer  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 
> > 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(_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,
> _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 

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

[James Almer]

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 
> 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(_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, 
> _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
> 
> 
>