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

[Vittorio Giovara]

On Thu, Feb 2, 2017 at 9:34 PM, James Almer <jamrial at 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 at gmail.com <mailto:vittorio.giovara at gmail.com>> wrote:
>>
>> On Sat, Jan 28, 2017 at 4:13 AM, James Almer <jamrial at gmail.com <mailto:jamrial at 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 at gmail.com <mailto:jamrial at 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.
-- 
Vittorio