[FFmpeg-devel] [PATCH 1/2] hevcdec: Miss the location of chroma samples when exporting stream parameters

Hendrik Leppkes h.leppkes at gmail.com
Thu May 17 13:30:00 EEST 2018


On Thu, May 17, 2018 at 8:08 AM, Xiang, Haihao <haihao.xiang at 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 at 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 at intel.com>
>> > > wrote:
>> > > > Signed-off-by: Haihao Xiang <haihao.xiang at 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


More information about the ffmpeg-devel mailing list