| Message ID | 20180817015257.7327-1-quinkblack@foxmail.com |
|---|---|
| State | Superseded |
| Headers | show |
On Fri, Aug 17, 2018 at 09:52:57AM +0800, Zhao Zhili wrote: > The values of SubWidthC and SubHeightC are 1 in the ITU-T H.265. The > current code use the value of 2. > --- > libavcodec/hevc_ps.c | 16 ++++++++++++---- > 1 file changed, 12 insertions(+), 4 deletions(-) > > diff --git a/libavcodec/hevc_ps.c b/libavcodec/hevc_ps.c > index fbd9fbf..b56b078 100644 > --- a/libavcodec/hevc_ps.c > +++ b/libavcodec/hevc_ps.c > @@ -70,6 +70,14 @@ static const AVRational vui_sar[] = { > { 2, 1 }, > }; > > +static const unsigned hevc_sub_width_c[] = { uint8_t saves a few bytes more important, the commit message should mention a ticket or test sample also a fate test with a _small_ testsample would be usefull. Obviously the existing tests do not cover this thx [...]
On 2018年08月18日 05:33, Michael Niedermayer wrote: > On Fri, Aug 17, 2018 at 09:52:57AM +0800, Zhao Zhili wrote: >> The values of SubWidthC and SubHeightC are 1 in the ITU-T H.265. The >> current code use the value of 2. >> --- >> libavcodec/hevc_ps.c | 16 ++++++++++++---- >> 1 file changed, 12 insertions(+), 4 deletions(-) >> >> diff --git a/libavcodec/hevc_ps.c b/libavcodec/hevc_ps.c >> index fbd9fbf..b56b078 100644 >> --- a/libavcodec/hevc_ps.c >> +++ b/libavcodec/hevc_ps.c >> @@ -70,6 +70,14 @@ static const AVRational vui_sar[] = { >> { 2, 1 }, >> }; >> >> +static const unsigned hevc_sub_width_c[] = { > uint8_t saves a few bytes > > more important, the commit message should mention a ticket or test sample > also a fate test with a _small_ testsample would be usefull. Obviously > the existing tests do not cover this > The bug was found by reading the source code. There is no ticket related to the bug. I need some time to download the test suite and figure out how it work. Feel free to add the test if anyone has a suitable sample. > thx > > [...] > > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
On Mon, Aug 20, 2018 at 10:19:04AM +0800, Zhao Zhili wrote: > > > On 2018年08月18日 05:33, Michael Niedermayer wrote: > >On Fri, Aug 17, 2018 at 09:52:57AM +0800, Zhao Zhili wrote: > >>The values of SubWidthC and SubHeightC are 1 in the ITU-T H.265. The > >>current code use the value of 2. > >>--- > >> libavcodec/hevc_ps.c | 16 ++++++++++++---- > >> 1 file changed, 12 insertions(+), 4 deletions(-) > >> > >>diff --git a/libavcodec/hevc_ps.c b/libavcodec/hevc_ps.c > >>index fbd9fbf..b56b078 100644 > >>--- a/libavcodec/hevc_ps.c > >>+++ b/libavcodec/hevc_ps.c > >>@@ -70,6 +70,14 @@ static const AVRational vui_sar[] = { > >> { 2, 1 }, > >> }; > >>+static const unsigned hevc_sub_width_c[] = { > >uint8_t saves a few bytes > > > >more important, the commit message should mention a ticket or test sample > >also a fate test with a _small_ testsample would be usefull. Obviously > >the existing tests do not cover this > > > > The bug was found by reading the source code. There is no ticket > related to the bug. I need some time to download the test suite > and figure out how it work. Feel free to add the test if anyone > has a suitable sample. if theres no test sample, creating one would be a good idea so this is tested. Because as is it would be a change that completely untested thx [...]
On 8/22/2018 6:40 AM, Zhao Zhili wrote: > > > On 2018年08月22日 08:26, Michael Niedermayer wrote: >> On Mon, Aug 20, 2018 at 10:19:04AM +0800, Zhao Zhili wrote: >>> >>> On 2018年08月18日 05:33, Michael Niedermayer wrote: >>>> On Fri, Aug 17, 2018 at 09:52:57AM +0800, Zhao Zhili wrote: >>>>> The values of SubWidthC and SubHeightC are 1 in the ITU-T H.265. The >>>>> current code use the value of 2. >>>>> --- >>>>> libavcodec/hevc_ps.c | 16 ++++++++++++---- >>>>> 1 file changed, 12 insertions(+), 4 deletions(-) >>>>> >>>>> diff --git a/libavcodec/hevc_ps.c b/libavcodec/hevc_ps.c >>>>> index fbd9fbf..b56b078 100644 >>>>> --- a/libavcodec/hevc_ps.c >>>>> +++ b/libavcodec/hevc_ps.c >>>>> @@ -70,6 +70,14 @@ static const AVRational vui_sar[] = { >>>>> { 2, 1 }, >>>>> }; >>>>> +static const unsigned hevc_sub_width_c[] = { >>>> uint8_t saves a few bytes >>>> >>>> more important, the commit message should mention a ticket or test >>>> sample >>>> also a fate test with a _small_ testsample would be usefull. Obviously >>>> the existing tests do not cover this >>>> >>> The bug was found by reading the source code. There is no ticket >>> related to the bug. I need some time to download the test suite >>> and figure out how it work. Feel free to add the test if anyone >>> has a suitable sample. >> if theres no test sample, creating one would be a good idea so this >> is tested. Because as is it would be a change that completely untested > > Updated patch and test sample are attached. Please review. > >> thx >> >> [...] >> >> >> _______________________________________________ >> ffmpeg-devel mailing list >> ffmpeg-devel@ffmpeg.org >> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > > 0001-lavc-hevc_ps-fix-crop-info-for-monochrome.patch > > > From 74dd822640b359beff6c843143e878fe39fc92c5 Mon Sep 17 00:00:00 2001 > From: Zhao Zhili <quinkblack@foxmail.com> > Date: Wed, 22 Aug 2018 17:37:15 +0800 > Subject: [PATCH] lavc/hevc_ps: fix crop info for monochrome > > The values of SubWidthC and SubHeightC are 1 in the ITU-T H.265. The > current code use the value of 2. > --- > libavcodec/hevc_ps.c | 16 ++++++++++++---- > tests/fate/hevc.mak | 3 +++ > tests/ref/fate/hevc-monochrome-crop | 6 ++++++ > 3 files changed, 21 insertions(+), 4 deletions(-) > create mode 100644 tests/ref/fate/hevc-monochrome-crop > > diff --git a/libavcodec/hevc_ps.c b/libavcodec/hevc_ps.c > index fbd9fbf..ea984af 100644 > --- a/libavcodec/hevc_ps.c > +++ b/libavcodec/hevc_ps.c > @@ -70,6 +70,14 @@ static const AVRational vui_sar[] = { > { 2, 1 }, > }; > > +static const uint8_t hevc_sub_width_c[] = { > + 1, 2, 2, 1 > +}; > + > +static const uint8_t hevc_sub_height_c[] = { > + 1, 2, 1, 1 > +}; > + > static void remove_pps(HEVCParamSets *s, int id) > { > if (s->pps_list[id] && s->pps == (const HEVCPPS*)s->pps_list[id]->data) > @@ -628,8 +636,8 @@ static void decode_vui(GetBitContext *gb, AVCodecContext *avctx, > vui->default_display_window_flag = get_bits1(gb); > > if (vui->default_display_window_flag) { > - int vert_mult = 1 + (sps->chroma_format_idc < 2); > - int horiz_mult = 1 + (sps->chroma_format_idc < 3); > + int vert_mult = hevc_sub_height_c[sps->chroma_format_idc]; > + int horiz_mult = hevc_sub_width_c[sps->chroma_format_idc]; > vui->def_disp_win.left_offset = get_ue_golomb_long(gb) * horiz_mult; > vui->def_disp_win.right_offset = get_ue_golomb_long(gb) * horiz_mult; > vui->def_disp_win.top_offset = get_ue_golomb_long(gb) * vert_mult; > @@ -923,8 +931,8 @@ int ff_hevc_parse_sps(HEVCSPS *sps, GetBitContext *gb, unsigned int *sps_id, > return ret; > > if (get_bits1(gb)) { // pic_conformance_flag > - int vert_mult = 1 + (sps->chroma_format_idc < 2); > - int horiz_mult = 1 + (sps->chroma_format_idc < 3); > + int vert_mult = hevc_sub_height_c[sps->chroma_format_idc]; > + int horiz_mult = hevc_sub_width_c[sps->chroma_format_idc]; > sps->pic_conf_win.left_offset = get_ue_golomb_long(gb) * horiz_mult; > sps->pic_conf_win.right_offset = get_ue_golomb_long(gb) * horiz_mult; > sps->pic_conf_win.top_offset = get_ue_golomb_long(gb) * vert_mult; > diff --git a/tests/fate/hevc.mak b/tests/fate/hevc.mak > index 184349e..9c288fc 100644 > --- a/tests/fate/hevc.mak > +++ b/tests/fate/hevc.mak > @@ -250,6 +250,9 @@ FATE_HEVC-$(call DEMDEC, MOV, HEVC) += fate-hevc-extradata-reload > > fate-hevc-extradata-reload: CMD = framemd5 -i $(TARGET_SAMPLES)/hevc/extradata-reload-multi-stsd.mov -sws_flags bitexact > > +fate-hevc-monochrome-crop: CMD = ffprobe$(PROGSSUF)$(EXESUF) -show_entries stream=width,height,coded_width,coded_height $(TARGET_SAMPLES)/hevc/hevc-monochrome.hevc This should have called run() before ffprobe. It fails otherwise as $(TARGET_PATH) is not added. Changed it to use the probeframes() function anyway so it may also probe the reported size of the frame in the stream, removed coded_width/height as those are deprecated and will be removed in an upcoming major version bump, and applied it. Thanks. > +FATE_HEVC_FFPROBE-$(call DEMDEC, HEVC, HEVC) += fate-hevc-monochrome-crop > + > FATE_SAMPLES_AVCONV += $(FATE_HEVC-yes) > FATE_SAMPLES_FFPROBE += $(FATE_HEVC_FFPROBE-yes) > > diff --git a/tests/ref/fate/hevc-monochrome-crop b/tests/ref/fate/hevc-monochrome-crop > new file mode 100644 > index 0000000..531bfc8 > --- /dev/null > +++ b/tests/ref/fate/hevc-monochrome-crop > @@ -0,0 +1,6 @@ > +[STREAM] > +width=384 > +height=240 > +coded_width=384 > +coded_height=256 > +[/STREAM] > -- 2.9.5 > > > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >
diff --git a/libavcodec/hevc_ps.c b/libavcodec/hevc_ps.c index fbd9fbf..b56b078 100644 --- a/libavcodec/hevc_ps.c +++ b/libavcodec/hevc_ps.c @@ -70,6 +70,14 @@ static const AVRational vui_sar[] = { { 2, 1 }, }; +static const unsigned hevc_sub_width_c[] = { + 1, 2, 2, 1 +}; + +static const unsigned hevc_sub_height_c[] = { + 1, 2, 1, 1 +}; + static void remove_pps(HEVCParamSets *s, int id) { if (s->pps_list[id] && s->pps == (const HEVCPPS*)s->pps_list[id]->data) @@ -628,8 +636,8 @@ static void decode_vui(GetBitContext *gb, AVCodecContext *avctx, vui->default_display_window_flag = get_bits1(gb); if (vui->default_display_window_flag) { - int vert_mult = 1 + (sps->chroma_format_idc < 2); - int horiz_mult = 1 + (sps->chroma_format_idc < 3); + int vert_mult = hevc_sub_height_c[sps->chroma_format_idc]; + int horiz_mult = hevc_sub_width_c[sps->chroma_format_idc]; vui->def_disp_win.left_offset = get_ue_golomb_long(gb) * horiz_mult; vui->def_disp_win.right_offset = get_ue_golomb_long(gb) * horiz_mult; vui->def_disp_win.top_offset = get_ue_golomb_long(gb) * vert_mult; @@ -923,8 +931,8 @@ int ff_hevc_parse_sps(HEVCSPS *sps, GetBitContext *gb, unsigned int *sps_id, return ret; if (get_bits1(gb)) { // pic_conformance_flag - int vert_mult = 1 + (sps->chroma_format_idc < 2); - int horiz_mult = 1 + (sps->chroma_format_idc < 3); + int vert_mult = hevc_sub_height_c[sps->chroma_format_idc]; + int horiz_mult = hevc_sub_width_c[sps->chroma_format_idc]; sps->pic_conf_win.left_offset = get_ue_golomb_long(gb) * horiz_mult; sps->pic_conf_win.right_offset = get_ue_golomb_long(gb) * horiz_mult; sps->pic_conf_win.top_offset = get_ue_golomb_long(gb) * vert_mult;