[FFmpeg-devel] [PATCH 1/3] spherical: Add tiled equirectangular type and projection-specific properties

[James Almer]

On 2/14/2017 11:20 PM, Vittorio Giovara wrote:
> On Tue, Feb 14, 2017 at 6:54 PM, James Almer <jamrial at gmail.com> wrote:
>> On 2/14/2017 5:52 PM, Vittorio Giovara wrote:
>>> On Fri, Feb 10, 2017 at 6:25 PM, Michael Niedermayer
>>> <michael at niedermayer.cc> wrote:
>>>> On Fri, Feb 10, 2017 at 04:11:43PM -0500, Vittorio Giovara wrote:
>>>>> Signed-off-by: Vittorio Giovara <vittorio.giovara at gmail.com>
>>>>> ---
>>>>> This should help not losing details over muxing and allows
>>>>> callers to get additional information in a clean manner.
>>>>>
>>>>> Please keep me in CC.
>>>>> Vittorio
>>>>>
>>>>>  doc/APIchanges        |  5 +++++
>>>>>  ffprobe.c             | 11 ++++++++--
>>>>>  libavformat/dump.c    | 10 +++++++++
>>>>>  libavutil/spherical.h | 56 +++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>  libavutil/version.h   |  2 +-
>>>>>  5 files changed, 81 insertions(+), 3 deletions(-)
>>>>
>>>> breaks fate
>>>>
>>>> --- ./tests/ref/fate/matroska-spherical-mono    2017-02-10 23:43:51.993432371 +0100
>>>> +++ tests/data/fate/matroska-spherical-mono     2017-02-11 00:24:10.297483318 +0100
>>>> @@ -7,7 +7,7 @@
>>>>  [/SIDE_DATA]
>>>>  [SIDE_DATA]
>>>>  side_data_type=Spherical Mapping
>>>> -side_data_size=16
>>>> +side_data_size=56
>>>>  projection=equirectangular
>>>>  yaw=45
>>>>  pitch=30
>>>> Test matroska-spherical-mono failed. Look at tests/data/fate/matroska-spherical-mono.err for details.
>>>> make: *** [fate-matroska-spherical-mono] Error 1
>>>
>>> Ah I didn't notice, it is fixed in the next commit, but I'll amend this one too.
>>>
>>>
>>> I didn't see any comment/discussion, should I assume it is ok?
>>> Please CC, thank you.
>>
>> These are a lot of projection specific fields. It worries me as the
>> spec may change in the future with new fields added or existing
>> fields changing purpose. Not to mention the Mesh projection, which
>> has like fifty specific fields of its own.
> 
> Even if the spec change (which at this point would be a terrible
> terrible thing to do) there are now files in the wild and software
> that have adopted this draft, so we would have to support this anyway.

If the spec changes, it will be the contents of the equi/cbmp/mesh.
By exporting them raw as extradata, said changes in the spec would
require no changes to our implementation.

> 
>> Wouldn't it be a better idea to export the binary data of the
>> equi/cbmp/mesh boxes into an extradata-like field in the
>> AVSphericalMapping struct, and let the downstream application parse
>> it instead?
> 
> No I don't think so, lavf is an abstraction layer and one of its tasks
> is to provide a (simple?) unified entry layer. and letting the user
> parse binary data is IMO bad design and very fragile. Also it's not
> impossible that another standard for tagging spherical metadata
> emerges in the future: the current API could very easily wrap it,
> while exporting the binary entry would be too specification-specific
> and it would be tied too heavily on the google draft.

AVSphericalMapping is already pretty tied to the google draft, but
i guess you're right, it's at least generic enough for now.

Wait for Aaron's opinion before addressing reviews and pushing. He
sent a different patchset himself and it wouldn't be nice to push
yours without at least giving him a chance to comment.