| Message ID | 20170418143012.5372-1-h.leppkes@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
Hi, On Tue, Apr 18, 2017 at 10:30 AM, Hendrik Leppkes <h.leppkes@gmail.com> wrote: > This brings our generation of the vpcC box up to date to the latest > draft version of the VP Codec ISO Media File Format Binding. > > Specifically, color/transfer properties are now written with values > based on ISO/IEC 23001-8, which is the same reference specification the > AVColor* enumerations are based on. > --- > libavformat/vpcc.c | 53 ++++-------------------------- > ----------------------- > 1 file changed, 4 insertions(+), 49 deletions(-) Cool! Ronald
On Tue, Apr 18, 2017 at 04:30:12PM +0200, Hendrik Leppkes wrote: > This brings our generation of the vpcC box up to date to the latest > draft version of the VP Codec ISO Media File Format Binding. > > Specifically, color/transfer properties are now written with values > based on ISO/IEC 23001-8, which is the same reference specification the > AVColor* enumerations are based on. > --- > libavformat/vpcc.c | 53 ++++------------------------------------------------- > 1 file changed, 4 insertions(+), 49 deletions(-) fails to build here: libavformat/vpcc.c: In function ‘ff_isom_write_vpcc’: libavformat/vpcc.c:81:26: error: ‘vpx_color_space’ undeclared (first use in this function) libavformat/vpcc.c:81:26: note: each undeclared identifier is reported only once for each function it appears in i assume iam missing some change ? [...]
On Tue, Apr 18, 2017 at 4:47 PM, Michael Niedermayer <michael@niedermayer.cc> wrote: > On Tue, Apr 18, 2017 at 04:30:12PM +0200, Hendrik Leppkes wrote: >> This brings our generation of the vpcC box up to date to the latest >> draft version of the VP Codec ISO Media File Format Binding. >> >> Specifically, color/transfer properties are now written with values >> based on ISO/IEC 23001-8, which is the same reference specification the >> AVColor* enumerations are based on. >> --- >> libavformat/vpcc.c | 53 ++++------------------------------------------------- >> 1 file changed, 4 insertions(+), 49 deletions(-) > > fails to build here: > > libavformat/vpcc.c: In function ‘ff_isom_write_vpcc’: > libavformat/vpcc.c:81:26: error: ‘vpx_color_space’ undeclared (first use in this function) > libavformat/vpcc.c:81:26: note: each undeclared identifier is reported only once for each function it appears in > > i assume iam missing some change ? > I failed to add a hunk to the patch, new one coming up. - Hendrik
On Tue, Apr 18, 2017 at 04:58:29PM +0200, Hendrik Leppkes wrote: > On Tue, Apr 18, 2017 at 4:47 PM, Michael Niedermayer > <michael@niedermayer.cc> wrote: > > On Tue, Apr 18, 2017 at 04:30:12PM +0200, Hendrik Leppkes wrote: > >> This brings our generation of the vpcC box up to date to the latest > >> draft version of the VP Codec ISO Media File Format Binding. > >> > >> Specifically, color/transfer properties are now written with values > >> based on ISO/IEC 23001-8, which is the same reference specification the > >> AVColor* enumerations are based on. > >> --- > >> libavformat/vpcc.c | 53 ++++------------------------------------------------- > >> 1 file changed, 4 insertions(+), 49 deletions(-) > > > > fails to build here: > > > > libavformat/vpcc.c: In function ‘ff_isom_write_vpcc’: > > libavformat/vpcc.c:81:26: error: ‘vpx_color_space’ undeclared (first use in this function) > > libavformat/vpcc.c:81:26: note: each undeclared identifier is reported only once for each function it appears in > > > > i assume iam missing some change ? > > > > I failed to add a hunk to the patch, new one coming up. confirmed, that fixes it thx [...]
Hi Hendrik, Thanks for working on it. Version needs to be updated as well: https://github.com/FFmpeg/FFmpeg/blob/b905ba5bc18c89c7fccd8621795755644442ef19/libavformat/movenc.c#L1115. Suggest moving it into ff_isom_write_vpcc function too. -- KongQun Yang (KQ) On Tue, Apr 18, 2017 at 7:30 AM, Hendrik Leppkes <h.leppkes@gmail.com> wrote: > This brings our generation of the vpcC box up to date to the latest > draft version of the VP Codec ISO Media File Format Binding. > > Specifically, color/transfer properties are now written with values > based on ISO/IEC 23001-8, which is the same reference specification the > AVColor* enumerations are based on. > --- > libavformat/vpcc.c | 53 ++++-------------------------- > ----------------------- > 1 file changed, 4 insertions(+), 49 deletions(-) > > diff --git a/libavformat/vpcc.c b/libavformat/vpcc.c > index 2390e1711c..d20ca9edc2 100644 > --- a/libavformat/vpcc.c > +++ b/libavformat/vpcc.c > @@ -23,44 +23,6 @@ > #include "libavutil/pixfmt.h" > #include "vpcc.h" > > -enum VpxColorSpace > -{ > - VPX_COLOR_SPACE_UNSPECIFIED = 0, > - VPX_COLOR_SPACE_BT601 = 1, > - VPX_COLOR_SPACE_BT709 = 2, > - VPX_COLOR_SPACE_SMPTE_170 = 3, > - VPX_COLOR_SPACE_SMPTE_240 = 4, > - VPX_COLOR_SPACE_BT2020_NCL = 5, > - VPX_COLOR_SPACE_BT2020_CL = 6, > - VPX_COLOR_SPACE_RGB = 7, > -}; > - > -static int get_vpx_color_space(AVFormatContext *s, > - enum AVColorSpace color_space) > -{ > - switch (color_space) { > - case AVCOL_SPC_RGB: > - return VPX_COLOR_SPACE_RGB; > - case AVCOL_SPC_BT709: > - return VPX_COLOR_SPACE_BT709; > - case AVCOL_SPC_UNSPECIFIED: > - return VPX_COLOR_SPACE_UNSPECIFIED; > - case AVCOL_SPC_BT470BG: > - return VPX_COLOR_SPACE_BT601; > - case AVCOL_SPC_SMPTE170M: > - return VPX_COLOR_SPACE_SMPTE_170; > - case AVCOL_SPC_SMPTE240M: > - return VPX_COLOR_SPACE_SMPTE_240; > - case AVCOL_SPC_BT2020_NCL: > - return VPX_COLOR_SPACE_BT2020_NCL; > - case AVCOL_SPC_BT2020_CL: > - return VPX_COLOR_SPACE_BT2020_CL; > - default: > - av_log(s, AV_LOG_ERROR, "Unsupported color space (%d)\n", > color_space); > - return -1; > - } > -} > - > enum VPX_CHROMA_SUBSAMPLING > { > VPX_SUBSAMPLING_420_VERTICAL = 0, > @@ -100,12 +62,6 @@ static int get_bit_depth(AVFormatContext *s, enum > AVPixelFormat pixel_format) > return desc->comp[0].depth; > } > > -static int get_vpx_transfer_function( > - enum AVColorTransferCharacteristic transfer) > -{ > - return transfer == AVCOL_TRC_SMPTEST2084; > -} > - > static int get_vpx_video_full_range_flag(enum AVColorRange color_range) > { > return color_range == AVCOL_RANGE_JPEG; > @@ -117,10 +73,8 @@ int ff_isom_write_vpcc(AVFormatContext *s, > AVIOContext *pb, > int profile = par->profile; > int level = par->level == FF_LEVEL_UNKNOWN ? 0 : par->level; > int bit_depth = get_bit_depth(s, par->format); > - int vpx_color_space = get_vpx_color_space(s, par->color_space); > int vpx_chroma_subsampling = > get_vpx_chroma_subsampling(s, par->format, par->chroma_location); > - int vpx_transfer_function = get_vpx_transfer_function(par- > >color_trc); > int vpx_video_full_range_flag = > get_vpx_video_full_range_flag(par->color_range); > > @@ -138,9 +92,10 @@ int ff_isom_write_vpcc(AVFormatContext *s, > AVIOContext *pb, > > avio_w8(pb, profile); > avio_w8(pb, level); > - avio_w8(pb, (bit_depth << 4) | vpx_color_space); > - avio_w8(pb, (vpx_chroma_subsampling << 4) | (vpx_transfer_function << > 1) | > - vpx_video_full_range_flag); > + avio_w8(pb, (bit_depth << 4) | (vpx_chroma_subsampling << 1) | > vpx_video_full_range_flag); > + avio_w8(pb, par->color_primaries); > + avio_w8(pb, par->color_trc); > + avio_w8(pb, par->color_space); > > // vp9 does not have codec initialization data. > avio_wb16(pb, 0); > -- > 2.12.2.windows.2 > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >
On Fri, Apr 21, 2017 at 11:25 PM, KongQun Yang <kqyang-at-google.com@ffmpeg.org> wrote: > Hi Hendrik, > > Thanks for working on it. Version needs to be updated as well: > https://github.com/FFmpeg/FFmpeg/blob/b905ba5bc18c89c7fccd8621795755644442ef19/libavformat/movenc.c#L1115. > Suggest moving it into ff_isom_write_vpcc function too. > The actual value of the version field doesn't seem to be mentioned in the specification anywhere. https://github.com/webmproject/vp9-dash/blob/master/VPCodecISOMediaFileFormatBinding.md The only version info I can see is the v1.0 at the top, but thats hardly a direct correlation to single integer version field in the mp4 box. Or did I miss it somewhere? - Hendrik
On 4/21/2017 6:34 PM, Hendrik Leppkes wrote: > On Fri, Apr 21, 2017 at 11:25 PM, KongQun Yang > <kqyang-at-google.com@ffmpeg.org> wrote: >> Hi Hendrik, >> >> Thanks for working on it. Version needs to be updated as well: >> https://github.com/FFmpeg/FFmpeg/blob/b905ba5bc18c89c7fccd8621795755644442ef19/libavformat/movenc.c#L1115. >> Suggest moving it into ff_isom_write_vpcc function too. >> > > The actual value of the version field doesn't seem to be mentioned in > the specification anywhere. > https://github.com/webmproject/vp9-dash/blob/master/VPCodecISOMediaFileFormatBinding.md > > The only version info I can see is the v1.0 at the top, but thats > hardly a direct correlation to single integer version field in the mp4 > box. > > Or did I miss it somewhere? > > - Hendrik https://github.com/webmproject/vp9-dash/commit/7961d0feb5bd879c84aa71d208d5df30bd5d5192
On 4/21/2017 6:47 PM, James Almer wrote: > On 4/21/2017 6:34 PM, Hendrik Leppkes wrote: >> On Fri, Apr 21, 2017 at 11:25 PM, KongQun Yang >> <kqyang-at-google.com@ffmpeg.org> wrote: >>> Hi Hendrik, >>> >>> Thanks for working on it. Version needs to be updated as well: >>> https://github.com/FFmpeg/FFmpeg/blob/b905ba5bc18c89c7fccd8621795755644442ef19/libavformat/movenc.c#L1115. >>> >>> Suggest moving it into ff_isom_write_vpcc function too. >>> >> >> The actual value of the version field doesn't seem to be mentioned in >> the specification anywhere. >> https://github.com/webmproject/vp9-dash/blob/master/VPCodecISOMediaFileFormatBinding.md >> >> >> The only version info I can see is the v1.0 at the top, but thats >> hardly a direct correlation to single integer version field in the mp4 >> box. >> >> Or did I miss it somewhere? >> >> - Hendrik > > https://github.com/webmproject/vp9-dash/commit/7961d0feb5bd879c84aa71d208d5df30bd5d5192 In fact, i think that commit is wrong. It changed the flags field from 0 to 1. As per the spec: aligned(8) class FullBox(unsigned int(32) boxtype, unsigned int(8) v, bit(24) f) extends Box(boxtype) { unsigned int(8) version = v; bit(24) flags = f; } So they are defining the version number as "version" in VPCodecISOMediaFileFormatBinding.md in any case, version is now 1 instead of 0.
Correct. There is a mistake in the spec. I have created a pull request to fix that earlier today: https://github.com/webmproject/vp9-dash/pull/69 -- KongQun Yang (KQ) On Fri, Apr 21, 2017 at 2:52 PM, James Almer <jamrial@gmail.com> wrote: > On 4/21/2017 6:47 PM, James Almer wrote: > >> On 4/21/2017 6:34 PM, Hendrik Leppkes wrote: >> >>> On Fri, Apr 21, 2017 at 11:25 PM, KongQun Yang >>> <kqyang-at-google.com@ffmpeg.org> wrote: >>> >>>> Hi Hendrik, >>>> >>>> Thanks for working on it. Version needs to be updated as well: >>>> https://github.com/FFmpeg/FFmpeg/blob/b905ba5bc18c89c7fccd86 >>>> 21795755644442ef19/libavformat/movenc.c#L1115. >>>> Suggest moving it into ff_isom_write_vpcc function too. >>>> >>>> >>> The actual value of the version field doesn't seem to be mentioned in >>> the specification anywhere. >>> https://github.com/webmproject/vp9-dash/blob/master/VPCodecI >>> SOMediaFileFormatBinding.md >>> >>> The only version info I can see is the v1.0 at the top, but thats >>> hardly a direct correlation to single integer version field in the mp4 >>> box. >>> >>> Or did I miss it somewhere? >>> >>> - Hendrik >>> >> >> https://github.com/webmproject/vp9-dash/commit/7961d0feb5bd8 >> 79c84aa71d208d5df30bd5d5192 >> > > In fact, i think that commit is wrong. It changed the flags field from 0 > to 1. As per the spec: > > aligned(8) class FullBox(unsigned int(32) boxtype, unsigned int(8) v, > bit(24) f) > extends Box(boxtype) { > unsigned int(8) version = v; > bit(24) flags = f; > } > > So they are defining the version number as "version" in > VPCodecISOMediaFileFormatBinding.md > > in any case, version is now 1 instead of 0. > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >
On Fri, Apr 21, 2017 at 11:52 PM, James Almer <jamrial@gmail.com> wrote: > On 4/21/2017 6:47 PM, James Almer wrote: >> >> On 4/21/2017 6:34 PM, Hendrik Leppkes wrote: >>> >>> On Fri, Apr 21, 2017 at 11:25 PM, KongQun Yang >>> <kqyang-at-google.com@ffmpeg.org> wrote: >>>> >>>> Hi Hendrik, >>>> >>>> Thanks for working on it. Version needs to be updated as well: >>>> >>>> https://github.com/FFmpeg/FFmpeg/blob/b905ba5bc18c89c7fccd8621795755644442ef19/libavformat/movenc.c#L1115. >>>> Suggest moving it into ff_isom_write_vpcc function too. >>>> >>> >>> The actual value of the version field doesn't seem to be mentioned in >>> the specification anywhere. >>> >>> https://github.com/webmproject/vp9-dash/blob/master/VPCodecISOMediaFileFormatBinding.md >>> >>> The only version info I can see is the v1.0 at the top, but thats >>> hardly a direct correlation to single integer version field in the mp4 >>> box. >>> >>> Or did I miss it somewhere? >>> >>> - Hendrik >> >> >> >> https://github.com/webmproject/vp9-dash/commit/7961d0feb5bd879c84aa71d208d5df30bd5d5192 > > > In fact, i think that commit is wrong. It changed the flags field from 0 to > 1. As per the spec: > > aligned(8) class FullBox(unsigned int(32) boxtype, unsigned int(8) v, > bit(24) f) > extends Box(boxtype) { > unsigned int(8) version = v; > bit(24) flags = f; > } > > So they are defining the version number as "version" in > VPCodecISOMediaFileFormatBinding.md > > in any case, version is now 1 instead of 0. > Yeah I looked up the FullBox spec and figured the flags of 1 might mean something I wasn't aware of. Anyhow, I'll send an amended patch soon that writes an appropriate version numer - Hendrik
diff --git a/libavformat/vpcc.c b/libavformat/vpcc.c index 2390e1711c..d20ca9edc2 100644 --- a/libavformat/vpcc.c +++ b/libavformat/vpcc.c @@ -23,44 +23,6 @@ #include "libavutil/pixfmt.h" #include "vpcc.h" -enum VpxColorSpace -{ - VPX_COLOR_SPACE_UNSPECIFIED = 0, - VPX_COLOR_SPACE_BT601 = 1, - VPX_COLOR_SPACE_BT709 = 2, - VPX_COLOR_SPACE_SMPTE_170 = 3, - VPX_COLOR_SPACE_SMPTE_240 = 4, - VPX_COLOR_SPACE_BT2020_NCL = 5, - VPX_COLOR_SPACE_BT2020_CL = 6, - VPX_COLOR_SPACE_RGB = 7, -}; - -static int get_vpx_color_space(AVFormatContext *s, - enum AVColorSpace color_space) -{ - switch (color_space) { - case AVCOL_SPC_RGB: - return VPX_COLOR_SPACE_RGB; - case AVCOL_SPC_BT709: - return VPX_COLOR_SPACE_BT709; - case AVCOL_SPC_UNSPECIFIED: - return VPX_COLOR_SPACE_UNSPECIFIED; - case AVCOL_SPC_BT470BG: - return VPX_COLOR_SPACE_BT601; - case AVCOL_SPC_SMPTE170M: - return VPX_COLOR_SPACE_SMPTE_170; - case AVCOL_SPC_SMPTE240M: - return VPX_COLOR_SPACE_SMPTE_240; - case AVCOL_SPC_BT2020_NCL: - return VPX_COLOR_SPACE_BT2020_NCL; - case AVCOL_SPC_BT2020_CL: - return VPX_COLOR_SPACE_BT2020_CL; - default: - av_log(s, AV_LOG_ERROR, "Unsupported color space (%d)\n", color_space); - return -1; - } -} - enum VPX_CHROMA_SUBSAMPLING { VPX_SUBSAMPLING_420_VERTICAL = 0, @@ -100,12 +62,6 @@ static int get_bit_depth(AVFormatContext *s, enum AVPixelFormat pixel_format) return desc->comp[0].depth; } -static int get_vpx_transfer_function( - enum AVColorTransferCharacteristic transfer) -{ - return transfer == AVCOL_TRC_SMPTEST2084; -} - static int get_vpx_video_full_range_flag(enum AVColorRange color_range) { return color_range == AVCOL_RANGE_JPEG; @@ -117,10 +73,8 @@ int ff_isom_write_vpcc(AVFormatContext *s, AVIOContext *pb, int profile = par->profile; int level = par->level == FF_LEVEL_UNKNOWN ? 0 : par->level; int bit_depth = get_bit_depth(s, par->format); - int vpx_color_space = get_vpx_color_space(s, par->color_space); int vpx_chroma_subsampling = get_vpx_chroma_subsampling(s, par->format, par->chroma_location); - int vpx_transfer_function = get_vpx_transfer_function(par->color_trc); int vpx_video_full_range_flag = get_vpx_video_full_range_flag(par->color_range); @@ -138,9 +92,10 @@ int ff_isom_write_vpcc(AVFormatContext *s, AVIOContext *pb, avio_w8(pb, profile); avio_w8(pb, level); - avio_w8(pb, (bit_depth << 4) | vpx_color_space); - avio_w8(pb, (vpx_chroma_subsampling << 4) | (vpx_transfer_function << 1) | - vpx_video_full_range_flag); + avio_w8(pb, (bit_depth << 4) | (vpx_chroma_subsampling << 1) | vpx_video_full_range_flag); + avio_w8(pb, par->color_primaries); + avio_w8(pb, par->color_trc); + avio_w8(pb, par->color_space); // vp9 does not have codec initialization data. avio_wb16(pb, 0);