| 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 | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
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
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
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 --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);
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(+)