diff mbox series

[FFmpeg-devel] lavf/mux: allow unofficial extension by default for spatial data in mp4

Message ID 20200827131217.23384-1-linjie.justin.fu@gmail.com
State New
Headers show
Series [FFmpeg-devel] lavf/mux: allow unofficial extension by default for spatial data in mp4
Related show

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Linjie Fu Aug. 27, 2020, 1:12 p.m. UTC
Mp4 unofficial extension allows Stereo3D and Spherical Mapping data
in header if users explicitly set "-strict unofficial" or values less
than "-1".

Currently side_data like stereo3D and spherical data in mp4 would be
dropped by default in a transcoding procedure even if user uses streamcopy
to get the same bitstreams. Spatial data missing in containers seems to
cause troubles for the players like VLC while detecting the projection type
for 360 video.

Set the default value of "strict" to "unofficial" for mp4.

Signed-off-by: Linjie Fu <linjie.justin.fu@gmail.com>
---
I would prefer to add the default value for format-specific options in something
like AVCodecDefault, however didn't find one.
 libavformat/mux.c | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Andreas Rheinhardt Aug. 28, 2020, 12:39 a.m. UTC | #1
Linjie Fu:
> Mp4 unofficial extension allows Stereo3D and Spherical Mapping data
> in header if users explicitly set "-strict unofficial" or values less
> than "-1".
> 
> Currently side_data like stereo3D and spherical data in mp4 would be
> dropped by default in a transcoding procedure even if user uses streamcopy
> to get the same bitstreams. Spatial data missing in containers seems to
> cause troubles for the players like VLC while detecting the projection type
> for 360 video.
> 
> Set the default value of "strict" to "unofficial" for mp4.
> 
> Signed-off-by: Linjie Fu <linjie.justin.fu@gmail.com>
> ---
> I would prefer to add the default value for format-specific options in something
> like AVCodecDefault, however didn't find one.
>  libavformat/mux.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/libavformat/mux.c b/libavformat/mux.c
> index 44d5e5d1c0..ffb9109a6f 100644
> --- a/libavformat/mux.c
> +++ b/libavformat/mux.c
> @@ -237,6 +237,11 @@ static int init_muxer(AVFormatContext *s, AVDictionary **options)
>      const AVCodecDescriptor *desc;
>      AVDictionaryEntry *e;
>  
> +    if (s->oformat) {
> +        if (!strcmp("mp4", s->oformat->name))
> +            s->strict_std_compliance = FF_COMPLIANCE_UNOFFICIAL;
> +    }
> +
>      if (options)
>          av_dict_copy(&tmp, *options, 0);
>  
1. s->oformat must already be set at this point.
2. You seem to believe that the only way for the user to set
strict_std_compliance etc. is by providing via the a dict provided to
avformat_init_output/write_header(). This is wrong. The user can also
set it before that. Your patch would break this.
Obviously you have no way to distinguish whether the user explicitly set
the option to FF_COMPLIANCE_NORMAL (and even less way to know whether
the user intentionally doesn't want unofficial extensions and therefore
opted to not touch strict_std_compliance).
3. One could print a warning in case the such unofficial extensions are
dropped.
3. How "unofficial" is this extension anyway? After all, the default
mode intentionally differs from FF_COMPLIANCE_STRICT.

- Andreas
Linjie Fu Aug. 28, 2020, 3:18 a.m. UTC | #2
Hi,

On Fri, Aug 28, 2020 at 8:46 AM Andreas Rheinhardt
<andreas.rheinhardt@gmail.com> wrote:
>
> Linjie Fu:
> > Mp4 unofficial extension allows Stereo3D and Spherical Mapping data
> > in header if users explicitly set "-strict unofficial" or values less
> > than "-1".
> >
> > Currently side_data like stereo3D and spherical data in mp4 would be
> > dropped by default in a transcoding procedure even if user uses streamcopy
> > to get the same bitstreams. Spatial data missing in containers seems to
> > cause troubles for the players like VLC while detecting the projection type
> > for 360 video.
> >
> > Set the default value of "strict" to "unofficial" for mp4.
> >
> > Signed-off-by: Linjie Fu <linjie.justin.fu@gmail.com>
> > ---
> > I would prefer to add the default value for format-specific options in something
> > like AVCodecDefault, however didn't find one.
> >  libavformat/mux.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/libavformat/mux.c b/libavformat/mux.c
> > index 44d5e5d1c0..ffb9109a6f 100644
> > --- a/libavformat/mux.c
> > +++ b/libavformat/mux.c
> > @@ -237,6 +237,11 @@ static int init_muxer(AVFormatContext *s, AVDictionary **options)
> >      const AVCodecDescriptor *desc;
> >      AVDictionaryEntry *e;
> >
> > +    if (s->oformat) {
> > +        if (!strcmp("mp4", s->oformat->name))
> > +            s->strict_std_compliance = FF_COMPLIANCE_UNOFFICIAL;
> > +    }
> > +
> >      if (options)
> >          av_dict_copy(&tmp, *options, 0);
> >
> 1. s->oformat must already be set at this point.
> 2. You seem to believe that the only way for the user to set
> strict_std_compliance etc. is by providing via the a dict provided to
> avformat_init_output/write_header(). This is wrong. The user can also
> set it before that. Your patch would break this.

It's like a priority order issue of:
1. user specified somehow in AVFormatContext;
2. user explicitly sets through options;
3. default value for specific container.

The logic for profile choosing in libopenh264enc seems to be good if we we had
a default value of something like "unknown" instead of "normal", since we can't
tell whether the "normal" is derived by user's choice or just a default value:
https://github.com/FFmpeg/FFmpeg/blob/master/libavcodec/libopenh264enc.c#L198

> Obviously you have no way to distinguish whether the user explicitly set
> the option to FF_COMPLIANCE_NORMAL (and even less way to know whether
> the user intentionally doesn't want unofficial extensions and therefore
> opted to not touch strict_std_compliance).

If I understood it correctly, user explicitly setting "strict" to
"normal" is able to be distinguished.
av_opt_set_dict() is able to replace the default "unofficial" with the
user specified "normal":
https://github.com/FFmpeg/FFmpeg/blob/master/libavformat/mux.c#L243

> 3. One could print a warning in case the such unofficial extensions are
> dropped.
Sure, a warning should be added.

> 3. How "unofficial" is this extension anyway? After all, the default
> mode intentionally differs from FF_COMPLIANCE_STRICT.

Defined as unofficial in 86dee47e397fe6bb0907adae8d4a54138a947646.

Seems to be defined in:
https://github.com/google/spatial-media/blob/master/docs/spherical-video-v2-rfc.md

which is also cited/referenced by youtube hellp with details for
adding metadata by ffmpeg:
https://support.google.com/youtube/answer/7278886?hl=en

- Linjie
Linjie Fu Aug. 28, 2020, 6:20 a.m. UTC | #3
On Fri, Aug 28, 2020 at 11:18 AM Linjie Fu <linjie.justin.fu@gmail.com> wrote:
>
> Hi,
>
> On Fri, Aug 28, 2020 at 8:46 AM Andreas Rheinhardt
> <andreas.rheinhardt@gmail.com> wrote:
> >
> > Linjie Fu:
> > > Mp4 unofficial extension allows Stereo3D and Spherical Mapping data
> > > in header if users explicitly set "-strict unofficial" or values less
> > > than "-1".
> > >
> > > Currently side_data like stereo3D and spherical data in mp4 would be
> > > dropped by default in a transcoding procedure even if user uses streamcopy
> > > to get the same bitstreams. Spatial data missing in containers seems to
> > > cause troubles for the players like VLC while detecting the projection type
> > > for 360 video.
> > >
> > > Set the default value of "strict" to "unofficial" for mp4.
> > >
> > > Signed-off-by: Linjie Fu <linjie.justin.fu@gmail.com>
> > > ---
> > > I would prefer to add the default value for format-specific options in something
> > > like AVCodecDefault, however didn't find one.
> > >  libavformat/mux.c | 5 +++++
> > >  1 file changed, 5 insertions(+)
> > >
> > > diff --git a/libavformat/mux.c b/libavformat/mux.c
> > > index 44d5e5d1c0..ffb9109a6f 100644
> > > --- a/libavformat/mux.c
> > > +++ b/libavformat/mux.c
> > > @@ -237,6 +237,11 @@ static int init_muxer(AVFormatContext *s, AVDictionary **options)
> > >      const AVCodecDescriptor *desc;
> > >      AVDictionaryEntry *e;
> > >
> > > +    if (s->oformat) {
> > > +        if (!strcmp("mp4", s->oformat->name))
> > > +            s->strict_std_compliance = FF_COMPLIANCE_UNOFFICIAL;
> > > +    }
> > > +
> > >      if (options)
> > >          av_dict_copy(&tmp, *options, 0);
> > >
> > 1. s->oformat must already be set at this point.
> > 2. You seem to believe that the only way for the user to set
> > strict_std_compliance etc. is by providing via the a dict provided to
> > avformat_init_output/write_header(). This is wrong. The user can also
> > set it before that. Your patch would break this.
>
> It's like a priority order issue of:
> 1. user specified somehow in AVFormatContext;
> 2. user explicitly sets through options;
> 3. default value for specific container.
>
> The logic for profile choosing in libopenh264enc seems to be good if we we had
> a default value of something like "unknown" instead of "normal", since we can't
> tell whether the "normal" is derived by user's choice or just a default value:
> https://github.com/FFmpeg/FFmpeg/blob/master/libavcodec/libopenh264enc.c#L198
>
> > Obviously you have no way to distinguish whether the user explicitly set
> > the option to FF_COMPLIANCE_NORMAL (and even less way to know whether
> > the user intentionally doesn't want unofficial extensions and therefore
> > opted to not touch strict_std_compliance).
>
> If I understood it correctly, user explicitly setting "strict" to
> "normal" is able to be distinguished.
> av_opt_set_dict() is able to replace the default "unofficial" with the
> user specified "normal":
> https://github.com/FFmpeg/FFmpeg/blob/master/libavformat/mux.c#L243
>
> > 3. One could print a warning in case the such unofficial extensions are
> > dropped.
> Sure, a warning should be added.
>
> > 3. How "unofficial" is this extension anyway? After all, the default
> > mode intentionally differs from FF_COMPLIANCE_STRICT.
>
> Defined as unofficial in 86dee47e397fe6bb0907adae8d4a54138a947646.
>
> Seems to be defined in:
> https://github.com/google/spatial-media/blob/master/docs/spherical-video-v2-rfc.md
>
> which is also cited/referenced by youtube hellp with details for
> adding metadata by ffmpeg:
> https://support.google.com/youtube/answer/7278886?hl=en
>

To make this easier to be reproduced/verified, attached the clips:
https://drive.google.com/file/d/1tU1HuPvCzOyYY3UBp2Eh7Xi8NWldczxD/view?usp=sharing

$ ffmpeg -i Professional+4K+360°+live+streaming+with+Insta360+Pro+and+Yi+360+VR.mp4
-c:v copy -an -y out.mp4
$ ffmpeg -i out.mp4
Then check the side data information.
diff mbox series

Patch

diff --git a/libavformat/mux.c b/libavformat/mux.c
index 44d5e5d1c0..ffb9109a6f 100644
--- a/libavformat/mux.c
+++ b/libavformat/mux.c
@@ -237,6 +237,11 @@  static int init_muxer(AVFormatContext *s, AVDictionary **options)
     const AVCodecDescriptor *desc;
     AVDictionaryEntry *e;
 
+    if (s->oformat) {
+        if (!strcmp("mp4", s->oformat->name))
+            s->strict_std_compliance = FF_COMPLIANCE_UNOFFICIAL;
+    }
+
     if (options)
         av_dict_copy(&tmp, *options, 0);