| Message ID | 20180516071945.28983-1-haihao.xiang@intel.com |
|---|---|
| State | New |
| Headers | show |
On Wed, May 16, 2018 at 9:19 AM, Haihao Xiang <haihao.xiang@intel.com> wrote: > Signed-off-by: Haihao Xiang <haihao.xiang@intel.com> > --- > libavcodec/hevcdec.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/libavcodec/hevcdec.c b/libavcodec/hevcdec.c > index c8877626d2..13d868bb4f 100644 > --- a/libavcodec/hevcdec.c > +++ b/libavcodec/hevcdec.c > @@ -344,6 +344,11 @@ static void export_stream_params(AVCodecContext *avctx, const HEVCParamSets *ps, > avctx->colorspace = AVCOL_SPC_UNSPECIFIED; > } > > + if (sps->vui.chroma_loc_info_present_flag) > + avctx->chroma_sample_location = sps->vui.chroma_sample_loc_type_top_field + 1; > + else > + avctx->chroma_sample_location = AVCHROMA_LOC_UNSPECIFIED; > This change is incomplete, refer to the patch I proposed earlier: http://ffmpeg.org/pipermail/ffmpeg-devel/2018-April/228100.html - Hendrik
On Wed, 2018-05-16 at 10:17 +0200, Hendrik Leppkes wrote: > On Wed, May 16, 2018 at 9:19 AM, Haihao Xiang <haihao.xiang@intel.com> wrote: > > Signed-off-by: Haihao Xiang <haihao.xiang@intel.com> > > --- > > libavcodec/hevcdec.c | 5 +++++ > > 1 file changed, 5 insertions(+) > > > > diff --git a/libavcodec/hevcdec.c b/libavcodec/hevcdec.c > > index c8877626d2..13d868bb4f 100644 > > --- a/libavcodec/hevcdec.c > > +++ b/libavcodec/hevcdec.c > > @@ -344,6 +344,11 @@ static void export_stream_params(AVCodecContext *avctx, > > const HEVCParamSets *ps, > > avctx->colorspace = AVCOL_SPC_UNSPECIFIED; > > } > > > > + if (sps->vui.chroma_loc_info_present_flag) > > + avctx->chroma_sample_location = sps- > > >vui.chroma_sample_loc_type_top_field + 1; > > + else > > + avctx->chroma_sample_location = AVCHROMA_LOC_UNSPECIFIED; > > > > This change is incomplete, refer to the patch I proposed earlier: > http://ffmpeg.org/pipermail/ffmpeg-devel/2018-April/228100.html > Sorry I didn't see your proposal before. Per spec, once chroma_loc_info_present_flag is set, chroma_format_idc should be 1. I think it would be better to add some checks when parsing the sequence parameters. Thanks Haihao > > - Hendrik > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
On Wed, May 16, 2018 at 10:49 AM, Xiang, Haihao <haihao.xiang@intel.com> wrote: > On Wed, 2018-05-16 at 10:17 +0200, Hendrik Leppkes wrote: >> On Wed, May 16, 2018 at 9:19 AM, Haihao Xiang <haihao.xiang@intel.com> wrote: >> > Signed-off-by: Haihao Xiang <haihao.xiang@intel.com> >> > --- >> > libavcodec/hevcdec.c | 5 +++++ >> > 1 file changed, 5 insertions(+) >> > >> > diff --git a/libavcodec/hevcdec.c b/libavcodec/hevcdec.c >> > index c8877626d2..13d868bb4f 100644 >> > --- a/libavcodec/hevcdec.c >> > +++ b/libavcodec/hevcdec.c >> > @@ -344,6 +344,11 @@ static void export_stream_params(AVCodecContext *avctx, >> > const HEVCParamSets *ps, >> > avctx->colorspace = AVCOL_SPC_UNSPECIFIED; >> > } >> > >> > + if (sps->vui.chroma_loc_info_present_flag) >> > + avctx->chroma_sample_location = sps- >> > >vui.chroma_sample_loc_type_top_field + 1; >> > + else >> > + avctx->chroma_sample_location = AVCHROMA_LOC_UNSPECIFIED; >> > >> >> This change is incomplete, refer to the patch I proposed earlier: >> http://ffmpeg.org/pipermail/ffmpeg-devel/2018-April/228100.html >> > > Sorry I didn't see your proposal before. Per spec, once > chroma_loc_info_present_flag is set, chroma_format_idc should be 1. I think it > would be better to add some checks when parsing the sequence parameters. > It makes no real difference because you still need the check to be able to set the LEFT default value for 4:2:0 only. - Hendrik
On Wed, 16 May 2018 15:19:44 +0800 Haihao Xiang <haihao.xiang@intel.com> wrote: > Signed-off-by: Haihao Xiang <haihao.xiang@intel.com> > --- > libavcodec/hevcdec.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/libavcodec/hevcdec.c b/libavcodec/hevcdec.c > index c8877626d2..13d868bb4f 100644 > --- a/libavcodec/hevcdec.c > +++ b/libavcodec/hevcdec.c > @@ -344,6 +344,11 @@ static void export_stream_params(AVCodecContext *avctx, const HEVCParamSets *ps, > avctx->colorspace = AVCOL_SPC_UNSPECIFIED; > } > > + if (sps->vui.chroma_loc_info_present_flag) > + avctx->chroma_sample_location = sps->vui.chroma_sample_loc_type_top_field + 1; > + else > + avctx->chroma_sample_location = AVCHROMA_LOC_UNSPECIFIED; > + > if (vps->vps_timing_info_present_flag) { > num = vps->vps_num_units_in_tick; > den = vps->vps_time_scale; Wouldn't this prevent an API user from setting the field to a container value as a fallback? Although maybe that's not necessary because there's an "unspecified" special value anyway.
On Wed, May 16, 2018 at 8:11 PM, wm4 <nfxjfg@googlemail.com> wrote: > On Wed, 16 May 2018 15:19:44 +0800 > Haihao Xiang <haihao.xiang@intel.com> wrote: > >> Signed-off-by: Haihao Xiang <haihao.xiang@intel.com> >> --- >> libavcodec/hevcdec.c | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/libavcodec/hevcdec.c b/libavcodec/hevcdec.c >> index c8877626d2..13d868bb4f 100644 >> --- a/libavcodec/hevcdec.c >> +++ b/libavcodec/hevcdec.c >> @@ -344,6 +344,11 @@ static void export_stream_params(AVCodecContext *avctx, const HEVCParamSets *ps, >> avctx->colorspace = AVCOL_SPC_UNSPECIFIED; >> } >> >> + if (sps->vui.chroma_loc_info_present_flag) >> + avctx->chroma_sample_location = sps->vui.chroma_sample_loc_type_top_field + 1; >> + else >> + avctx->chroma_sample_location = AVCHROMA_LOC_UNSPECIFIED; >> + >> if (vps->vps_timing_info_present_flag) { >> num = vps->vps_num_units_in_tick; >> den = vps->vps_time_scale; > > Wouldn't this prevent an API user from setting the field to a container > value as a fallback? Although maybe that's not necessary because > there's an "unspecified" special value anyway. The decoder always manages that value, if the SPS/PPS/VPS changed mid-stream and the value vanishes, that also needs to be able to be communicated. - Hendrik
On Wed, 2018-05-16 at 11:27 +0200, Hendrik Leppkes wrote: > On Wed, May 16, 2018 at 10:49 AM, Xiang, Haihao <haihao.xiang@intel.com> > wrote: > > On Wed, 2018-05-16 at 10:17 +0200, Hendrik Leppkes wrote: > > > On Wed, May 16, 2018 at 9:19 AM, Haihao Xiang <haihao.xiang@intel.com> > > > wrote: > > > > Signed-off-by: Haihao Xiang <haihao.xiang@intel.com> > > > > --- > > > > libavcodec/hevcdec.c | 5 +++++ > > > > 1 file changed, 5 insertions(+) > > > > > > > > diff --git a/libavcodec/hevcdec.c b/libavcodec/hevcdec.c > > > > index c8877626d2..13d868bb4f 100644 > > > > --- a/libavcodec/hevcdec.c > > > > +++ b/libavcodec/hevcdec.c > > > > @@ -344,6 +344,11 @@ static void export_stream_params(AVCodecContext > > > > *avctx, > > > > const HEVCParamSets *ps, > > > > avctx->colorspace = AVCOL_SPC_UNSPECIFIED; > > > > } > > > > > > > > + if (sps->vui.chroma_loc_info_present_flag) > > > > + avctx->chroma_sample_location = sps- > > > > > vui.chroma_sample_loc_type_top_field + 1; > > > > > > > > + else > > > > + avctx->chroma_sample_location = AVCHROMA_LOC_UNSPECIFIED; > > > > > > > > > > This change is incomplete, refer to the patch I proposed earlier: > > > http://ffmpeg.org/pipermail/ffmpeg-devel/2018-April/228100.html > > > > > > > Sorry I didn't see your proposal before. Per spec, once > > chroma_loc_info_present_flag is set, chroma_format_idc should be 1. I think > > it > > would be better to add some checks when parsing the sequence parameters. > > > > It makes no real difference because you still need the check to be > able to set the LEFT default value for 4:2:0 only. If chroma_sample_loc_type_top_field is set correctly when parsing SPS, we may set chroma_sample_loc_type_top_field + 1 for 4:2:0 below no matter the present flag if (sps->chroma_format_idc == 1) avctx->chroma_sample_location = sps->vui.chroma_sample_loc_type_top_field + 1; else avctx->chroma_sample_location = AVCHROMA_LOC_UNSPECIFIED; Thanks Haihao > > - Hendrik > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
On Thu, May 17, 2018 at 8:08 AM, Xiang, Haihao <haihao.xiang@intel.com> wrote: > On Wed, 2018-05-16 at 11:27 +0200, Hendrik Leppkes wrote: >> On Wed, May 16, 2018 at 10:49 AM, Xiang, Haihao <haihao.xiang@intel.com> >> wrote: >> > On Wed, 2018-05-16 at 10:17 +0200, Hendrik Leppkes wrote: >> > > On Wed, May 16, 2018 at 9:19 AM, Haihao Xiang <haihao.xiang@intel.com> >> > > wrote: >> > > > Signed-off-by: Haihao Xiang <haihao.xiang@intel.com> >> > > > --- >> > > > libavcodec/hevcdec.c | 5 +++++ >> > > > 1 file changed, 5 insertions(+) >> > > > >> > > > diff --git a/libavcodec/hevcdec.c b/libavcodec/hevcdec.c >> > > > index c8877626d2..13d868bb4f 100644 >> > > > --- a/libavcodec/hevcdec.c >> > > > +++ b/libavcodec/hevcdec.c >> > > > @@ -344,6 +344,11 @@ static void export_stream_params(AVCodecContext >> > > > *avctx, >> > > > const HEVCParamSets *ps, >> > > > avctx->colorspace = AVCOL_SPC_UNSPECIFIED; >> > > > } >> > > > >> > > > + if (sps->vui.chroma_loc_info_present_flag) >> > > > + avctx->chroma_sample_location = sps- >> > > > > vui.chroma_sample_loc_type_top_field + 1; >> > > > >> > > > + else >> > > > + avctx->chroma_sample_location = AVCHROMA_LOC_UNSPECIFIED; >> > > > >> > > >> > > This change is incomplete, refer to the patch I proposed earlier: >> > > http://ffmpeg.org/pipermail/ffmpeg-devel/2018-April/228100.html >> > > >> > >> > Sorry I didn't see your proposal before. Per spec, once >> > chroma_loc_info_present_flag is set, chroma_format_idc should be 1. I think >> > it >> > would be better to add some checks when parsing the sequence parameters. >> > >> >> It makes no real difference because you still need the check to be >> able to set the LEFT default value for 4:2:0 only. > > If chroma_sample_loc_type_top_field is set correctly when parsing SPS, we may > set chroma_sample_loc_type_top_field + 1 for 4:2:0 below no matter the present > flag > > if (sps->chroma_format_idc == 1) > avctx->chroma_sample_location = sps->vui.chroma_sample_loc_type_top_field + > 1; > else > avctx->chroma_sample_location = AVCHROMA_LOC_UNSPECIFIED; > The fields in the sps struct should reflect the bitstream, interpretation should be left out of it. Like mentioned above, you always need the same checks anyway, all you do is move it into another place. It is IMHO much cleaner to keep the interpretation of all those values in the export_stream_params function, its not like its complex or anything, but it cleanly seperates parsing and interpretation, and ensures the sps struct only contains values directly read from the bitstream. - Hendrik
On Thu, 2018-05-17 at 12:30 +0200, Hendrik Leppkes wrote: > On Thu, May 17, 2018 at 8:08 AM, Xiang, Haihao <haihao.xiang@intel.com> wrote: > > On Wed, 2018-05-16 at 11:27 +0200, Hendrik Leppkes wrote: > > > On Wed, May 16, 2018 at 10:49 AM, Xiang, Haihao <haihao.xiang@intel.com> > > > wrote: > > > > On Wed, 2018-05-16 at 10:17 +0200, Hendrik Leppkes wrote: > > > > > On Wed, May 16, 2018 at 9:19 AM, Haihao Xiang <haihao.xiang@intel.com> > > > > > wrote: > > > > > > Signed-off-by: Haihao Xiang <haihao.xiang@intel.com> > > > > > > --- > > > > > > libavcodec/hevcdec.c | 5 +++++ > > > > > > 1 file changed, 5 insertions(+) > > > > > > > > > > > > diff --git a/libavcodec/hevcdec.c b/libavcodec/hevcdec.c > > > > > > index c8877626d2..13d868bb4f 100644 > > > > > > --- a/libavcodec/hevcdec.c > > > > > > +++ b/libavcodec/hevcdec.c > > > > > > @@ -344,6 +344,11 @@ static void export_stream_params(AVCodecContext > > > > > > *avctx, > > > > > > const HEVCParamSets *ps, > > > > > > avctx->colorspace = AVCOL_SPC_UNSPECIFIED; > > > > > > } > > > > > > > > > > > > + if (sps->vui.chroma_loc_info_present_flag) > > > > > > + avctx->chroma_sample_location = sps- > > > > > > > vui.chroma_sample_loc_type_top_field + 1; > > > > > > > > > > > > + else > > > > > > + avctx->chroma_sample_location = AVCHROMA_LOC_UNSPECIFIED; > > > > > > > > > > > > > > > > This change is incomplete, refer to the patch I proposed earlier: > > > > > http://ffmpeg.org/pipermail/ffmpeg-devel/2018-April/228100.html > > > > > > > > > > > > > Sorry I didn't see your proposal before. Per spec, once > > > > chroma_loc_info_present_flag is set, chroma_format_idc should be 1. I > > > > think > > > > it > > > > would be better to add some checks when parsing the sequence parameters. > > > > > > > > > > It makes no real difference because you still need the check to be > > > able to set the LEFT default value for 4:2:0 only. > > > > If chroma_sample_loc_type_top_field is set correctly when parsing SPS, we > > may > > set chroma_sample_loc_type_top_field + 1 for 4:2:0 below no matter the > > present > > flag > > > > if (sps->chroma_format_idc == 1) > > avctx->chroma_sample_location = sps- > > >vui.chroma_sample_loc_type_top_field + > > 1; > > else > > avctx->chroma_sample_location = AVCHROMA_LOC_UNSPECIFIED; > > > > The fields in the sps struct should reflect the bitstream, > interpretation should be left out of it. > Actually the checks for some field are done in sps, e.g. def_disp_win_left_offset/def_disp_win_right_offset/def_disp_win_top_offset/ def_disp_win_bottom_offset. User needn't worry about the values when using these fields. > Like mentioned above, you always need the same checks anyway, all you > do is move it into another place. It is IMHO much cleaner to keep the > interpretation of all those values in the export_stream_params > function, its not like its complex or anything, but it cleanly > seperates parsing and interpretation, and ensures the sps struct only > contains values directly read from the bitstream. > > - Hendrik > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
On Fri, May 18, 2018 at 10:52 AM, Xiang, Haihao <haihao.xiang@intel.com> wrote: > On Thu, 2018-05-17 at 12:30 +0200, Hendrik Leppkes wrote: >> On Thu, May 17, 2018 at 8:08 AM, Xiang, Haihao <haihao.xiang@intel.com> wrote: >> > On Wed, 2018-05-16 at 11:27 +0200, Hendrik Leppkes wrote: >> > > On Wed, May 16, 2018 at 10:49 AM, Xiang, Haihao <haihao.xiang@intel.com> >> > > wrote: >> > > > On Wed, 2018-05-16 at 10:17 +0200, Hendrik Leppkes wrote: >> > > > > On Wed, May 16, 2018 at 9:19 AM, Haihao Xiang <haihao.xiang@intel.com> >> > > > > wrote: >> > > > > > Signed-off-by: Haihao Xiang <haihao.xiang@intel.com> >> > > > > > --- >> > > > > > libavcodec/hevcdec.c | 5 +++++ >> > > > > > 1 file changed, 5 insertions(+) >> > > > > > >> > > > > > diff --git a/libavcodec/hevcdec.c b/libavcodec/hevcdec.c >> > > > > > index c8877626d2..13d868bb4f 100644 >> > > > > > --- a/libavcodec/hevcdec.c >> > > > > > +++ b/libavcodec/hevcdec.c >> > > > > > @@ -344,6 +344,11 @@ static void export_stream_params(AVCodecContext >> > > > > > *avctx, >> > > > > > const HEVCParamSets *ps, >> > > > > > avctx->colorspace = AVCOL_SPC_UNSPECIFIED; >> > > > > > } >> > > > > > >> > > > > > + if (sps->vui.chroma_loc_info_present_flag) >> > > > > > + avctx->chroma_sample_location = sps- >> > > > > > > vui.chroma_sample_loc_type_top_field + 1; >> > > > > > >> > > > > > + else >> > > > > > + avctx->chroma_sample_location = AVCHROMA_LOC_UNSPECIFIED; >> > > > > > >> > > > > >> > > > > This change is incomplete, refer to the patch I proposed earlier: >> > > > > http://ffmpeg.org/pipermail/ffmpeg-devel/2018-April/228100.html >> > > > > >> > > > >> > > > Sorry I didn't see your proposal before. Per spec, once >> > > > chroma_loc_info_present_flag is set, chroma_format_idc should be 1. I >> > > > think >> > > > it >> > > > would be better to add some checks when parsing the sequence parameters. >> > > > >> > > >> > > It makes no real difference because you still need the check to be >> > > able to set the LEFT default value for 4:2:0 only. >> > >> > If chroma_sample_loc_type_top_field is set correctly when parsing SPS, we >> > may >> > set chroma_sample_loc_type_top_field + 1 for 4:2:0 below no matter the >> > present >> > flag >> > >> > if (sps->chroma_format_idc == 1) >> > avctx->chroma_sample_location = sps- >> > >vui.chroma_sample_loc_type_top_field + >> > 1; >> > else >> > avctx->chroma_sample_location = AVCHROMA_LOC_UNSPECIFIED; >> > >> >> The fields in the sps struct should reflect the bitstream, >> interpretation should be left out of it. >> > > Actually the checks for some field are done in sps, e.g. > def_disp_win_left_offset/def_disp_win_right_offset/def_disp_win_top_offset/ > def_disp_win_bottom_offset. User needn't worry about the values when using these > fields. > > These fields are not available to "users". - Hendrik
On Fri, 2018-05-18 at 11:13 +0200, Hendrik Leppkes wrote: > On Fri, May 18, 2018 at 10:52 AM, Xiang, Haihao <haihao.xiang@intel.com> > wrote: > > On Thu, 2018-05-17 at 12:30 +0200, Hendrik Leppkes wrote: > > > On Thu, May 17, 2018 at 8:08 AM, Xiang, Haihao <haihao.xiang@intel.com> > > > wrote: > > > > On Wed, 2018-05-16 at 11:27 +0200, Hendrik Leppkes wrote: > > > > > On Wed, May 16, 2018 at 10:49 AM, Xiang, Haihao <haihao.xiang@intel.co > > > > > m> > > > > > wrote: > > > > > > On Wed, 2018-05-16 at 10:17 +0200, Hendrik Leppkes wrote: > > > > > > > On Wed, May 16, 2018 at 9:19 AM, Haihao Xiang <haihao.xiang@intel. > > > > > > > com> > > > > > > > wrote: > > > > > > > > Signed-off-by: Haihao Xiang <haihao.xiang@intel.com> > > > > > > > > --- > > > > > > > > libavcodec/hevcdec.c | 5 +++++ > > > > > > > > 1 file changed, 5 insertions(+) > > > > > > > > > > > > > > > > diff --git a/libavcodec/hevcdec.c b/libavcodec/hevcdec.c > > > > > > > > index c8877626d2..13d868bb4f 100644 > > > > > > > > --- a/libavcodec/hevcdec.c > > > > > > > > +++ b/libavcodec/hevcdec.c > > > > > > > > @@ -344,6 +344,11 @@ static void > > > > > > > > export_stream_params(AVCodecContext > > > > > > > > *avctx, > > > > > > > > const HEVCParamSets *ps, > > > > > > > > avctx->colorspace = AVCOL_SPC_UNSPECIFIED; > > > > > > > > } > > > > > > > > > > > > > > > > + if (sps->vui.chroma_loc_info_present_flag) > > > > > > > > + avctx->chroma_sample_location = sps- > > > > > > > > > vui.chroma_sample_loc_type_top_field + 1; > > > > > > > > > > > > > > > > + else > > > > > > > > + avctx->chroma_sample_location = > > > > > > > > AVCHROMA_LOC_UNSPECIFIED; > > > > > > > > > > > > > > > > > > > > > > This change is incomplete, refer to the patch I proposed earlier: > > > > > > > http://ffmpeg.org/pipermail/ffmpeg-devel/2018-April/228100.html > > > > > > > > > > > > > > > > > > > Sorry I didn't see your proposal before. Per spec, once > > > > > > chroma_loc_info_present_flag is set, chroma_format_idc should be 1. > > > > > > I > > > > > > think > > > > > > it > > > > > > would be better to add some checks when parsing the sequence > > > > > > parameters. > > > > > > > > > > > > > > > > It makes no real difference because you still need the check to be > > > > > able to set the LEFT default value for 4:2:0 only. > > > > > > > > If chroma_sample_loc_type_top_field is set correctly when parsing SPS, > > > > we > > > > may > > > > set chroma_sample_loc_type_top_field + 1 for 4:2:0 below no matter the > > > > present > > > > flag > > > > > > > > if (sps->chroma_format_idc == 1) > > > > avctx->chroma_sample_location = sps- > > > > > vui.chroma_sample_loc_type_top_field + > > > > > > > > 1; > > > > else > > > > avctx->chroma_sample_location = AVCHROMA_LOC_UNSPECIFIED; > > > > > > > > > > The fields in the sps struct should reflect the bitstream, > > > interpretation should be left out of it. > > > > > > > Actually the checks for some field are done in sps, e.g. > > def_disp_win_left_offset/def_disp_win_right_offset/def_disp_win_top_offset/ > > def_disp_win_bottom_offset. User needn't worry about the values when using > > these > > fields. > > > > > > These fields are not available to "users". Per the current implementation, sps->vui.def_disp_win may impact avctx- >width/avctx->height, so I mean these fields may be used. BTW setting sps->vui.chroma_sample_loc_type_top_field to 0 for 4:2:0 in sps structure should reflect the bitstream, why do you think it is interpretation? > > - Hendrik > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
On Thu, May 24, 2018 at 8:22 AM, Xiang, Haihao <haihao.xiang@intel.com> wrote: > On Fri, 2018-05-18 at 11:13 +0200, Hendrik Leppkes wrote: >> On Fri, May 18, 2018 at 10:52 AM, Xiang, Haihao <haihao.xiang@intel.com> >> wrote: >> > On Thu, 2018-05-17 at 12:30 +0200, Hendrik Leppkes wrote: >> > > On Thu, May 17, 2018 at 8:08 AM, Xiang, Haihao <haihao.xiang@intel.com> >> > > wrote: >> > > > On Wed, 2018-05-16 at 11:27 +0200, Hendrik Leppkes wrote: >> > > > > On Wed, May 16, 2018 at 10:49 AM, Xiang, Haihao <haihao.xiang@intel.co >> > > > > m> >> > > > > wrote: >> > > > > > On Wed, 2018-05-16 at 10:17 +0200, Hendrik Leppkes wrote: >> > > > > > > On Wed, May 16, 2018 at 9:19 AM, Haihao Xiang <haihao.xiang@intel. >> > > > > > > com> >> > > > > > > wrote: >> > > > > > > > Signed-off-by: Haihao Xiang <haihao.xiang@intel.com> >> > > > > > > > --- >> > > > > > > > libavcodec/hevcdec.c | 5 +++++ >> > > > > > > > 1 file changed, 5 insertions(+) >> > > > > > > > >> > > > > > > > diff --git a/libavcodec/hevcdec.c b/libavcodec/hevcdec.c >> > > > > > > > index c8877626d2..13d868bb4f 100644 >> > > > > > > > --- a/libavcodec/hevcdec.c >> > > > > > > > +++ b/libavcodec/hevcdec.c >> > > > > > > > @@ -344,6 +344,11 @@ static void >> > > > > > > > export_stream_params(AVCodecContext >> > > > > > > > *avctx, >> > > > > > > > const HEVCParamSets *ps, >> > > > > > > > avctx->colorspace = AVCOL_SPC_UNSPECIFIED; >> > > > > > > > } >> > > > > > > > >> > > > > > > > + if (sps->vui.chroma_loc_info_present_flag) >> > > > > > > > + avctx->chroma_sample_location = sps- >> > > > > > > > > vui.chroma_sample_loc_type_top_field + 1; >> > > > > > > > >> > > > > > > > + else >> > > > > > > > + avctx->chroma_sample_location = >> > > > > > > > AVCHROMA_LOC_UNSPECIFIED; >> > > > > > > > >> > > > > > > >> > > > > > > This change is incomplete, refer to the patch I proposed earlier: >> > > > > > > http://ffmpeg.org/pipermail/ffmpeg-devel/2018-April/228100.html >> > > > > > > >> > > > > > >> > > > > > Sorry I didn't see your proposal before. Per spec, once >> > > > > > chroma_loc_info_present_flag is set, chroma_format_idc should be 1. >> > > > > > I >> > > > > > think >> > > > > > it >> > > > > > would be better to add some checks when parsing the sequence >> > > > > > parameters. >> > > > > > >> > > > > >> > > > > It makes no real difference because you still need the check to be >> > > > > able to set the LEFT default value for 4:2:0 only. >> > > > >> > > > If chroma_sample_loc_type_top_field is set correctly when parsing SPS, >> > > > we >> > > > may >> > > > set chroma_sample_loc_type_top_field + 1 for 4:2:0 below no matter the >> > > > present >> > > > flag >> > > > >> > > > if (sps->chroma_format_idc == 1) >> > > > avctx->chroma_sample_location = sps- >> > > > > vui.chroma_sample_loc_type_top_field + >> > > > >> > > > 1; >> > > > else >> > > > avctx->chroma_sample_location = AVCHROMA_LOC_UNSPECIFIED; >> > > > >> > > >> > > The fields in the sps struct should reflect the bitstream, >> > > interpretation should be left out of it. >> > > >> > >> > Actually the checks for some field are done in sps, e.g. >> > def_disp_win_left_offset/def_disp_win_right_offset/def_disp_win_top_offset/ >> > def_disp_win_bottom_offset. User needn't worry about the values when using >> > these >> > fields. >> > >> > >> >> These fields are not available to "users". > > Per the current implementation, sps->vui.def_disp_win may impact avctx- >>width/avctx->height, so I mean these fields may be used. > > BTW setting sps->vui.chroma_sample_loc_type_top_field to 0 for 4:2:0 in sps > structure should reflect the bitstream, why do you think it is interpretation? > Is the value coded in the bitstream? If not, then its interpretation. - Hendrik
On Thu, 2018-05-24 at 10:07 +0200, Hendrik Leppkes wrote: > On Thu, May 24, 2018 at 8:22 AM, Xiang, Haihao <haihao.xiang@intel.com> wrote: > > On Fri, 2018-05-18 at 11:13 +0200, Hendrik Leppkes wrote: > > > On Fri, May 18, 2018 at 10:52 AM, Xiang, Haihao <haihao.xiang@intel.com> > > > wrote: > > > > On Thu, 2018-05-17 at 12:30 +0200, Hendrik Leppkes wrote: > > > > > On Thu, May 17, 2018 at 8:08 AM, Xiang, Haihao <haihao.xiang@intel.com > > > > > > > > > > > wrote: > > > > > > On Wed, 2018-05-16 at 11:27 +0200, Hendrik Leppkes wrote: > > > > > > > On Wed, May 16, 2018 at 10:49 AM, Xiang, Haihao <haihao.xiang@inte > > > > > > > l.co > > > > > > > m> > > > > > > > wrote: > > > > > > > > On Wed, 2018-05-16 at 10:17 +0200, Hendrik Leppkes wrote: > > > > > > > > > On Wed, May 16, 2018 at 9:19 AM, Haihao Xiang <haihao.xiang@in > > > > > > > > > tel. > > > > > > > > > com> > > > > > > > > > wrote: > > > > > > > > > > Signed-off-by: Haihao Xiang <haihao.xiang@intel.com> > > > > > > > > > > --- > > > > > > > > > > libavcodec/hevcdec.c | 5 +++++ > > > > > > > > > > 1 file changed, 5 insertions(+) > > > > > > > > > > > > > > > > > > > > diff --git a/libavcodec/hevcdec.c b/libavcodec/hevcdec.c > > > > > > > > > > index c8877626d2..13d868bb4f 100644 > > > > > > > > > > --- a/libavcodec/hevcdec.c > > > > > > > > > > +++ b/libavcodec/hevcdec.c > > > > > > > > > > @@ -344,6 +344,11 @@ static void > > > > > > > > > > export_stream_params(AVCodecContext > > > > > > > > > > *avctx, > > > > > > > > > > const HEVCParamSets *ps, > > > > > > > > > > avctx->colorspace = AVCOL_SPC_UNSPECIFIED; > > > > > > > > > > } > > > > > > > > > > > > > > > > > > > > + if (sps->vui.chroma_loc_info_present_flag) > > > > > > > > > > + avctx->chroma_sample_location = sps- > > > > > > > > > > > vui.chroma_sample_loc_type_top_field + 1; > > > > > > > > > > > > > > > > > > > > + else > > > > > > > > > > + avctx->chroma_sample_location = > > > > > > > > > > AVCHROMA_LOC_UNSPECIFIED; > > > > > > > > > > > > > > > > > > > > > > > > > > > > This change is incomplete, refer to the patch I proposed > > > > > > > > > earlier: > > > > > > > > > http://ffmpeg.org/pipermail/ffmpeg-devel/2018-April/228100.htm > > > > > > > > > l > > > > > > > > > > > > > > > > > > > > > > > > > Sorry I didn't see your proposal before. Per spec, once > > > > > > > > chroma_loc_info_present_flag is set, chroma_format_idc should be > > > > > > > > 1. > > > > > > > > I > > > > > > > > think > > > > > > > > it > > > > > > > > would be better to add some checks when parsing the sequence > > > > > > > > parameters. > > > > > > > > > > > > > > > > > > > > > > It makes no real difference because you still need the check to be > > > > > > > able to set the LEFT default value for 4:2:0 only. > > > > > > > > > > > > If chroma_sample_loc_type_top_field is set correctly when parsing > > > > > > SPS, > > > > > > we > > > > > > may > > > > > > set chroma_sample_loc_type_top_field + 1 for 4:2:0 below no matter > > > > > > the > > > > > > present > > > > > > flag > > > > > > > > > > > > if (sps->chroma_format_idc == 1) > > > > > > avctx->chroma_sample_location = sps- > > > > > > > vui.chroma_sample_loc_type_top_field + > > > > > > > > > > > > 1; > > > > > > else > > > > > > avctx->chroma_sample_location = AVCHROMA_LOC_UNSPECIFIED; > > > > > > > > > > > > > > > > The fields in the sps struct should reflect the bitstream, > > > > > interpretation should be left out of it. > > > > > > > > > > > > > Actually the checks for some field are done in sps, e.g. > > > > def_disp_win_left_offset/def_disp_win_right_offset/def_disp_win_top_offs > > > > et/ > > > > def_disp_win_bottom_offset. User needn't worry about the values when > > > > using > > > > these > > > > fields. > > > > > > > > > > > > > > These fields are not available to "users". > > > > Per the current implementation, sps->vui.def_disp_win may impact avctx- > > > width/avctx->height, so I mean these fields may be used. > > > > BTW setting sps->vui.chroma_sample_loc_type_top_field to 0 for 4:2:0 in sps > > structure should reflect the bitstream, why do you think it is > > interpretation? > > > > Is the value coded in the bitstream? If not, then its interpretation. > If so, it seems there are lots of interpretation in sps. For example, the checks below are used for colour properties in sps: // Set invalid values to "unspecified" if (!av_color_primaries_name(vui->colour_primaries)) vui->colour_primaries = AVCOL_PRI_UNSPECIFIED; if (!av_color_transfer_name(vui->transfer_characteristic)) vui->transfer_characteristic = AVCOL_TRC_UNSPECIFIED; if (!av_color_space_name(vui->matrix_coeffs)) vui->matrix_coeffs = AVCOL_SPC_UNSPECIFIED; Do you think we should remove the above code from sps? I think it will be inconvenient to use the colour properties if we don't check the values in sps. Thanks Haihao
diff --git a/libavcodec/hevcdec.c b/libavcodec/hevcdec.c index c8877626d2..13d868bb4f 100644 --- a/libavcodec/hevcdec.c +++ b/libavcodec/hevcdec.c @@ -344,6 +344,11 @@ static void export_stream_params(AVCodecContext *avctx, const HEVCParamSets *ps, avctx->colorspace = AVCOL_SPC_UNSPECIFIED; } + if (sps->vui.chroma_loc_info_present_flag) + avctx->chroma_sample_location = sps->vui.chroma_sample_loc_type_top_field + 1; + else + avctx->chroma_sample_location = AVCHROMA_LOC_UNSPECIFIED; + if (vps->vps_timing_info_present_flag) { num = vps->vps_num_units_in_tick; den = vps->vps_time_scale;
Signed-off-by: Haihao Xiang <haihao.xiang@intel.com> --- libavcodec/hevcdec.c | 5 +++++ 1 file changed, 5 insertions(+)