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

[Aaron Colwell]

On Wed, Feb 15, 2017 at 5:52 AM James Almer <jamrial at gmail.com> wrote:

> 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.
>

This is one of the main reasons I was suggesting this path. I think of
these extradata fields much like the extra data that codecs have. It really
is only important to the code that needs to render a specific projection.
For transcoding, you mainly just need to convey the information in a
lossless manner from demuxer to muxer.

I anticipate the spec will change in the future. My plan is that no change
will break what is currently specified in the spec right now, but I
anticipate some changes will be made. Having a solution that can gracefully
handle this would be nice.



> >
> >> 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.
>
>
I agree with Vittorio that having some form of abstraction is a good thing
and having binary data in places can be problematic. It feels like we could
find some middle ground here by providing helper functions that parse the
binary data into projection specific structs and back just like codecs code
tends to do. I feel like this provides a reasonable balance between having
a common set of fields where things actually have common semantics like
projection_type, yaw/pitch/roll, & extra_data while also providing a way to
get access to projection specific information in a simple way.

At the end of the day players really just need to care about a rendering
mesh so in some sense it would be nice to have that be the abstraction for
the player use case. That is basically what we have done in our internal
player implementations. That could easily be handled by helper functions,
but would be a bad representation for AVSphericalMapping because it would
make transcoding/transmuxing painful.


> AVSphericalMapping is already pretty tied to the google draft, but
> i guess you're right, it's at least generic enough for now.
>
>
I too feel like this path is tying the API pretty closely to the Google
draft's semantics. This seems ok for things like signalling projection type
and yaw/pitch/roll projection rotation, but it seems a little risky for
projection parameterization. All you have to do is look at MPEGs OMAF
drafts to see that there are potentially many more projections to
potentially have to deal with and their parameters may not be useful
outside of each particular projection. That is why I'm wary of the "union
of all projection config fields" type of approach.



> 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.
>
>
Thank you. I really appreciate this. I hope it is clear I'm really trying
to find some middle ground here. My team has been working on this stuff
inside Google for several years and I'm just trying to bring some of that
experience and learning into the open here. I defer to you because this is
your project and I am relatively sparse contributor.

Aaron