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

Submitted by Haihao Xiang on May 16, 2018, 7:19 a.m.

Details

Message ID 20180516071945.28983-1-haihao.xiang@intel.com
State New
Headers show

Commit Message

Haihao Xiang May 16, 2018, 7:19 a.m.
Signed-off-by: Haihao Xiang <haihao.xiang@intel.com>
---
 libavcodec/hevcdec.c | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Hendrik Leppkes May 16, 2018, 8:17 a.m.
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
Haihao Xiang May 16, 2018, 8:49 a.m.
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
Hendrik Leppkes May 16, 2018, 9:27 a.m.
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
wm4 May 16, 2018, 6:11 p.m.
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.
Hendrik Leppkes May 16, 2018, 6:35 p.m.
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
Haihao Xiang May 17, 2018, 6:08 a.m.
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
Hendrik Leppkes May 17, 2018, 10:30 a.m.
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
Haihao Xiang May 18, 2018, 8:52 a.m.
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
Hendrik Leppkes May 18, 2018, 9:13 a.m.
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

Patch hide | download patch | download mbox

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;