On Thu, May 17, 2018 at 8:08 AM, Xiang, Haihao <haihao.xi...@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.xi...@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.xi...@intel.com> >> > > wrote: >> > > > Signed-off-by: Haihao Xiang <haihao.xi...@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 _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
