| Message ID | 1587258489-17189-3-git-send-email-mypopydev@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Support Dolby Vision | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
I'd like to ask opinions whether a installed header for just one structure is a good idea. On Sun, Apr 19, 2020, at 03:07, Jun Zhao wrote: > From: vacingfang <vacingfang@tencent.com> > > add DOVI related struct > > Signed-off-by: vacingfang <vacingfang@tencent.com> > --- > libavutil/Makefile | 1 + > libavutil/dovi_meta.h | 58 +++++++++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 59 insertions(+) > create mode 100644 libavutil/dovi_meta.h > > diff --git a/libavutil/Makefile b/libavutil/Makefile > index 8feb029..1aac84c 100644 > --- a/libavutil/Makefile > +++ b/libavutil/Makefile > @@ -23,6 +23,7 @@ HEADERS = adler32.h > \ > des.h > \ > dict.h > \ > display.h > \ > + dovi_meta.h > \ > downmix_info.h > \ > encryption_info.h > \ > error.h > \ > diff --git a/libavutil/dovi_meta.h b/libavutil/dovi_meta.h > new file mode 100644 > index 0000000..51c0a48 > --- /dev/null > +++ b/libavutil/dovi_meta.h > @@ -0,0 +1,58 @@ > +/* > + * Copyright (c) 2020 Vacing Fang <vacingfang@tencent.com> > + * > + * This file is part of FFmpeg. > + * > + * FFmpeg is free software; you can redistribute it and/or > + * modify it under the terms of the GNU Lesser General Public > + * License as published by the Free Software Foundation; either > + * version 2.1 of the License, or (at your option) any later version. > + * > + * FFmpeg is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + * Lesser General Public License for more details. > + * > + * You should have received a copy of the GNU Lesser General Public > + * License along with FFmpeg; if not, write to the Free Software > + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA > 02110-1301 USA > + */ > + > +/** > + * @file > + * DOVI configuration > + */ > + > + > +#ifndef AVUTIL_DOVI_META_H > +#define AVUTIL_DOVI_META_H > + > +#include <stdint.h> > + > +/* > + * DOVI configuration > + * ref: > dolby-vision-bitstreams-within-the-iso-base-media-file-format-v2.1.2 > + > dolby-vision-bitstreams-in-mpeg-2-transport-stream-multiplex-v1.2 > + * @code > + * uint8_t dv_version_major, the major version number that the stream > complies with > + * uint8_t dv_version_minor, the minor version number that the stream > complies with > + * uint8_t dv_profile, the Dolby Vision profile > + * uint8_t dv_level, the Dolby Vision level > + * uint8_t rpu_present_flag > + * uint8_t el_present_flag > + * uint8_t bl_present_flag > + * uint8_t dv_bl_signal_compatibility_id > + * @endcode > + */ > +typedef struct { > + uint8_t dv_version_major; > + uint8_t dv_version_minor; > + uint8_t dv_profile; > + uint8_t dv_level; > + uint8_t rpu_present_flag; > + uint8_t el_present_flag; > + uint8_t bl_present_flag; > + uint8_t dv_bl_signal_compatibility_id; > +} AVDOVIDecoderConfigurationRecord; > + > +#endif /* AVUTIL_DOVI_META_H */ > -- > 2.7.4 > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
On Sun, Apr 19, 2020 at 4:25 PM Jean-Baptiste Kempf <jb@videolan.org> wrote: > > I'd like to ask opinions whether a installed header for just one structure is a good idea. > It's follow the style like AVReplayGain, so add a new file in libavutil > On Sun, Apr 19, 2020, at 03:07, Jun Zhao wrote: > > From: vacingfang <vacingfang@tencent.com> > > > > add DOVI related struct > > > > Signed-off-by: vacingfang <vacingfang@tencent.com> > > --- > > libavutil/Makefile | 1 + > > libavutil/dovi_meta.h | 58 +++++++++++++++++++++++++++++++++++++++++++++++++++ > > 2 files changed, 59 insertions(+) > > create mode 100644 libavutil/dovi_meta.h > > > > diff --git a/libavutil/Makefile b/libavutil/Makefile > > index 8feb029..1aac84c 100644 > > --- a/libavutil/Makefile > > +++ b/libavutil/Makefile > > @@ -23,6 +23,7 @@ HEADERS = adler32.h > > \ > > des.h > > \ > > dict.h > > \ > > display.h > > \ > > + dovi_meta.h > > \ > > downmix_info.h > > \ > > encryption_info.h > > \ > > error.h > > \ > > diff --git a/libavutil/dovi_meta.h b/libavutil/dovi_meta.h > > new file mode 100644 > > index 0000000..51c0a48 > > --- /dev/null > > +++ b/libavutil/dovi_meta.h > > @@ -0,0 +1,58 @@ > > +/* > > + * Copyright (c) 2020 Vacing Fang <vacingfang@tencent.com> > > + * > > + * This file is part of FFmpeg. > > + * > > + * FFmpeg is free software; you can redistribute it and/or > > + * modify it under the terms of the GNU Lesser General Public > > + * License as published by the Free Software Foundation; either > > + * version 2.1 of the License, or (at your option) any later version. > > + * > > + * FFmpeg is distributed in the hope that it will be useful, > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > > + * Lesser General Public License for more details. > > + * > > + * You should have received a copy of the GNU Lesser General Public > > + * License along with FFmpeg; if not, write to the Free Software > > + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA > > 02110-1301 USA > > + */ > > + > > +/** > > + * @file > > + * DOVI configuration > > + */ > > + > > + > > +#ifndef AVUTIL_DOVI_META_H > > +#define AVUTIL_DOVI_META_H > > + > > +#include <stdint.h> > > + > > +/* > > + * DOVI configuration > > + * ref: > > dolby-vision-bitstreams-within-the-iso-base-media-file-format-v2.1.2 > > + > > dolby-vision-bitstreams-in-mpeg-2-transport-stream-multiplex-v1.2 > > + * @code > > + * uint8_t dv_version_major, the major version number that the stream > > complies with > > + * uint8_t dv_version_minor, the minor version number that the stream > > complies with > > + * uint8_t dv_profile, the Dolby Vision profile > > + * uint8_t dv_level, the Dolby Vision level > > + * uint8_t rpu_present_flag > > + * uint8_t el_present_flag > > + * uint8_t bl_present_flag > > + * uint8_t dv_bl_signal_compatibility_id > > + * @endcode > > + */ > > +typedef struct { > > + uint8_t dv_version_major; > > + uint8_t dv_version_minor; > > + uint8_t dv_profile; > > + uint8_t dv_level; > > + uint8_t rpu_present_flag; > > + uint8_t el_present_flag; > > + uint8_t bl_present_flag; > > + uint8_t dv_bl_signal_compatibility_id; > > +} AVDOVIDecoderConfigurationRecord; > > + > > +#endif /* AVUTIL_DOVI_META_H */ > > -- > > 2.7.4 > > > > _______________________________________________ > > ffmpeg-devel mailing list > > ffmpeg-devel@ffmpeg.org > > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > > > To unsubscribe, visit link above, or email > > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". > > -- > Jean-Baptiste Kempf - President > +33 672 704 734 > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Quoting Jean-Baptiste Kempf (2020-04-19 10:25:07)
> I'd like to ask opinions whether a installed header for just one structure is a good idea.
I see no problem with it. More smaller headers is better that a few big
headers.
Another issue is that this struct has no constructor and thus cannot be
extended without breaking ABI.
On Mon, Apr 20, 2020 at 9:08 PM Anton Khirnov <anton@khirnov.net> wrote: > > Quoting Jean-Baptiste Kempf (2020-04-19 10:25:07) > > I'd like to ask opinions whether a installed header for just one structure is a good idea. > > I see no problem with it. More smaller headers is better that a few big > headers. > > Another issue is that this struct has no constructor and thus cannot be > extended without breaking ABI. > This structure is part of API, so I can't find a simple way to extend without break the ABI, I believe there are similar problems for AVSphericalMapping
Quoting mypopy@gmail.com (2020-04-21 04:47:42) > On Mon, Apr 20, 2020 at 9:08 PM Anton Khirnov <anton@khirnov.net> wrote: > > > > Quoting Jean-Baptiste Kempf (2020-04-19 10:25:07) > > > I'd like to ask opinions whether a installed header for just one structure is a good idea. > > > > I see no problem with it. More smaller headers is better that a few big > > headers. > > > > Another issue is that this struct has no constructor and thus cannot be > > extended without breaking ABI. > > > This structure is part of API, so I can't find a simple way to extend > without break the ABI, I believe there are similar problems for > AVSphericalMapping No, AVSphericalMapping does not have that problem since it has a constructor.
On Tue, Apr 21, 2020 at 2:23 PM Anton Khirnov <anton@khirnov.net> wrote: > > Quoting mypopy@gmail.com (2020-04-21 04:47:42) > > On Mon, Apr 20, 2020 at 9:08 PM Anton Khirnov <anton@khirnov.net> wrote: > > > > > > Quoting Jean-Baptiste Kempf (2020-04-19 10:25:07) > > > > I'd like to ask opinions whether a installed header for just one structure is a good idea. > > > > > > I see no problem with it. More smaller headers is better that a few big > > > headers. > > > > > > Another issue is that this struct has no constructor and thus cannot be > > > extended without breaking ABI. > > > > > This structure is part of API, so I can't find a simple way to extend > > without break the ABI, I believe there are similar problems for > > AVSphericalMapping > > No, AVSphericalMapping does not have that problem since it has a > constructor. > Will add a constructor av_dovi_alloc with static inline in the header for this comment
Quoting mypopy@gmail.com (2020-04-22 05:39:38) > On Tue, Apr 21, 2020 at 2:23 PM Anton Khirnov <anton@khirnov.net> wrote: > > > > Quoting mypopy@gmail.com (2020-04-21 04:47:42) > > > On Mon, Apr 20, 2020 at 9:08 PM Anton Khirnov <anton@khirnov.net> wrote: > > > > > > > > Quoting Jean-Baptiste Kempf (2020-04-19 10:25:07) > > > > > I'd like to ask opinions whether a installed header for just one structure is a good idea. > > > > > > > > I see no problem with it. More smaller headers is better that a few big > > > > headers. > > > > > > > > Another issue is that this struct has no constructor and thus cannot be > > > > extended without breaking ABI. > > > > > > > This structure is part of API, so I can't find a simple way to extend > > > without break the ABI, I believe there are similar problems for > > > AVSphericalMapping > > > > No, AVSphericalMapping does not have that problem since it has a > > constructor. > > > Will add a constructor av_dovi_alloc with static inline in the header > for this comment Making it inline would invalidate the whole point, which is to avoid embedding the size of the struct into the calling program.
On Wed, Apr 22, 2020 at 4:19 PM Anton Khirnov <anton@khirnov.net> wrote: > > Quoting mypopy@gmail.com (2020-04-22 05:39:38) > > On Tue, Apr 21, 2020 at 2:23 PM Anton Khirnov <anton@khirnov.net> wrote: > > > > > > Quoting mypopy@gmail.com (2020-04-21 04:47:42) > > > > On Mon, Apr 20, 2020 at 9:08 PM Anton Khirnov <anton@khirnov.net> wrote: > > > > > > > > > > Quoting Jean-Baptiste Kempf (2020-04-19 10:25:07) > > > > > > I'd like to ask opinions whether a installed header for just one structure is a good idea. > > > > > > > > > > I see no problem with it. More smaller headers is better that a few big > > > > > headers. > > > > > > > > > > Another issue is that this struct has no constructor and thus cannot be > > > > > extended without breaking ABI. > > > > > > > > > This structure is part of API, so I can't find a simple way to extend > > > > without break the ABI, I believe there are similar problems for > > > > AVSphericalMapping > > > > > > No, AVSphericalMapping does not have that problem since it has a > > > constructor. > > > > > Will add a constructor av_dovi_alloc with static inline in the header > > for this comment > > Making it inline would invalidate the whole point, which is to avoid > embedding the size of the struct into the calling program. > I see, so please help to review https://patchwork.ffmpeg.org/project/ffmpeg/patch/1587530154-27795-3-git-send-email-mypopydev@gmail.com/
On Wed, Apr 22, 2020 at 4:19 PM Anton Khirnov <anton@khirnov.net> wrote: > > Quoting mypopy@gmail.com (2020-04-22 05:39:38) > > On Tue, Apr 21, 2020 at 2:23 PM Anton Khirnov <anton@khirnov.net> wrote: > > > > > > Quoting mypopy@gmail.com (2020-04-21 04:47:42) > > > > On Mon, Apr 20, 2020 at 9:08 PM Anton Khirnov <anton@khirnov.net> wrote: > > > > > > > > > > Quoting Jean-Baptiste Kempf (2020-04-19 10:25:07) > > > > > > I'd like to ask opinions whether a installed header for just one structure is a good idea. > > > > > > > > > > I see no problem with it. More smaller headers is better that a few big > > > > > headers. > > > > > > > > > > Another issue is that this struct has no constructor and thus cannot be > > > > > extended without breaking ABI. > > > > > > > > > This structure is part of API, so I can't find a simple way to extend > > > > without break the ABI, I believe there are similar problems for > > > > AVSphericalMapping > > > > > > No, AVSphericalMapping does not have that problem since it has a > > > constructor. > > > > > Will add a constructor av_dovi_alloc with static inline in the header > > for this comment > > Making it inline would invalidate the whole point, which is to avoid > embedding the size of the struct into the calling program. > Do you mean I need to split the av_dovi_alloc constructor to a separate c file?
Quoting mypopy@gmail.com (2020-04-22 10:56:52) > On Wed, Apr 22, 2020 at 4:19 PM Anton Khirnov <anton@khirnov.net> wrote: > > Making it inline would invalidate the whole point, which is to avoid > > embedding the size of the struct into the calling program. > > > Do you mean I need to split the av_dovi_alloc constructor to a > separate c file? Yes, the code has to be located in libavutil. So that if the struct is enlarged in some later version of libavutil, the calling programs will still work without having to be recompiled.
On Wed, Apr 22, 2020 at 7:55 PM Anton Khirnov <anton@khirnov.net> wrote: > > Quoting mypopy@gmail.com (2020-04-22 10:56:52) > > On Wed, Apr 22, 2020 at 4:19 PM Anton Khirnov <anton@khirnov.net> wrote: > > > Making it inline would invalidate the whole point, which is to avoid > > > embedding the size of the struct into the calling program. > > > > > Do you mean I need to split the av_dovi_alloc constructor to a > > separate c file? > > Yes, the code has to be located in libavutil. So that if the struct is > enlarged in some later version of libavutil, the calling programs will > still work without having to be recompiled. > Update the V6 patchset :)
diff --git a/libavutil/Makefile b/libavutil/Makefile index 8feb029..1aac84c 100644 --- a/libavutil/Makefile +++ b/libavutil/Makefile @@ -23,6 +23,7 @@ HEADERS = adler32.h \ des.h \ dict.h \ display.h \ + dovi_meta.h \ downmix_info.h \ encryption_info.h \ error.h \ diff --git a/libavutil/dovi_meta.h b/libavutil/dovi_meta.h new file mode 100644 index 0000000..51c0a48 --- /dev/null +++ b/libavutil/dovi_meta.h @@ -0,0 +1,58 @@ +/* + * Copyright (c) 2020 Vacing Fang <vacingfang@tencent.com> + * + * This file is part of FFmpeg. + * + * FFmpeg is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * FFmpeg is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with FFmpeg; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA + */ + +/** + * @file + * DOVI configuration + */ + + +#ifndef AVUTIL_DOVI_META_H +#define AVUTIL_DOVI_META_H + +#include <stdint.h> + +/* + * DOVI configuration + * ref: dolby-vision-bitstreams-within-the-iso-base-media-file-format-v2.1.2 + dolby-vision-bitstreams-in-mpeg-2-transport-stream-multiplex-v1.2 + * @code + * uint8_t dv_version_major, the major version number that the stream complies with + * uint8_t dv_version_minor, the minor version number that the stream complies with + * uint8_t dv_profile, the Dolby Vision profile + * uint8_t dv_level, the Dolby Vision level + * uint8_t rpu_present_flag + * uint8_t el_present_flag + * uint8_t bl_present_flag + * uint8_t dv_bl_signal_compatibility_id + * @endcode + */ +typedef struct { + uint8_t dv_version_major; + uint8_t dv_version_minor; + uint8_t dv_profile; + uint8_t dv_level; + uint8_t rpu_present_flag; + uint8_t el_present_flag; + uint8_t bl_present_flag; + uint8_t dv_bl_signal_compatibility_id; +} AVDOVIDecoderConfigurationRecord; + +#endif /* AVUTIL_DOVI_META_H */