[FFmpeg-devel] lavc/hevc_ps: fix crop info

Message ID	20180816032909.27915-1-quinkblack@foxmail.com
State	New
Headers	show Delivered-To: ffmpegpatchwork@gmail.com Received-SPF: pass (google.com: domain of ffmpeg-devel-bounces@ffmpeg.org designates 79.124.17.100 as permitted sender) client-ip=79.124.17.100; From: Zhao Zhili <quinkblack@foxmail.com> To: ffmpeg-devel@ffmpeg.org Date: Thu, 16 Aug 2018 11:29:09 +0800 Message-Id: <20180816032909.27915-1-quinkblack@foxmail.com> Feedback-ID: esmtp:foxmail.com:bgforeign:bgforeign1 Subject: [FFmpeg-devel] [PATCH] lavc/hevc_ps: fix crop info Precedence: list Reply-To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org> Cc: Zhao Zhili <quinkblack@foxmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" <ffmpeg-devel-bounces@ffmpeg.org>

Message ID

20180816032909.27915-1-quinkblack@foxmail.com

State

New

Headers

Received-SPF: pass (google.com: domain of ffmpeg-devel-bounces@ffmpeg.org
	designates 79.124.17.100 as permitted sender)
	client-ip=79.124.17.100; 
From: Zhao Zhili <quinkblack@foxmail.com>
To: ffmpeg-devel@ffmpeg.org
Date: Thu, 16 Aug 2018 11:29:09 +0800
Message-Id: <20180816032909.27915-1-quinkblack@foxmail.com>
Feedback-ID: esmtp:foxmail.com:bgforeign:bgforeign1
Subject: [FFmpeg-devel] [PATCH] lavc/hevc_ps: fix crop info
Precedence: list
Reply-To: FFmpeg development discussions and patches
	<ffmpeg-devel@ffmpeg.org>
Cc: Zhao Zhili <quinkblack@foxmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: base64
Errors-To: ffmpeg-devel-bounces@ffmpeg.org
Sender: "ffmpeg-devel" <ffmpeg-devel-bounces@ffmpeg.org>

Comments

James Almer Aug. 16, 2018, 3:46 a.m. UTC | #1

On 8/16/2018 12:29 AM, Zhao Zhili wrote:
> ---
>  libavcodec/hevc_ps.c | 26 ++++++++++++++++++++++----
>  1 file changed, 22 insertions(+), 4 deletions(-)
> 
> diff --git a/libavcodec/hevc_ps.c b/libavcodec/hevc_ps.c
> index fbd9fbf..4161ab6 100644
> --- a/libavcodec/hevc_ps.c
> +++ b/libavcodec/hevc_ps.c
> @@ -628,8 +628,17 @@ 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);
> +        unsigned horiz_mult, vert_mult;
> +        if (sps->chroma_format_idc == 1) {
> +            horiz_mult = 2;
> +            vert_mult = 2;
> +        } else if (sps->chroma_format_idc == 2) {
> +            horiz_mult = 2;
> +            vert_mult = 1;
> +        } else {
> +            horiz_mult = 1;
> +            vert_mult = 1;

Can you explain what this patch is meant to do? For chroma_format_idc ==
1, 2 and 3 nothing changes with this, only chroma_format_idc == 0, where
both horiz_mult and vert_mult become 1 instead of 2. Is that
correct/intended?

> +        }
>          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 +932,17 @@ 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);
> +        unsigned horiz_mult, vert_mult;
> +        if (sps->chroma_format_idc == 1) {
> +            horiz_mult = 2;
> +            vert_mult = 2;
> +        } else if (sps->chroma_format_idc == 2) {
> +            horiz_mult = 2;
> +            vert_mult = 1;
> +        } else {
> +            horiz_mult = 1;
> +            vert_mult = 1;
> +        }
>          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;
>

Zhao Zhili Aug. 16, 2018, 5 a.m. UTC | #2

On 2018年08月16日 11:46, James Almer wrote:
> On 8/16/2018 12:29 AM, Zhao Zhili wrote:
>> ---
>>   libavcodec/hevc_ps.c | 26 ++++++++++++++++++++++----
>>   1 file changed, 22 insertions(+), 4 deletions(-)
>>
>> diff --git a/libavcodec/hevc_ps.c b/libavcodec/hevc_ps.c
>> index fbd9fbf..4161ab6 100644
>> --- a/libavcodec/hevc_ps.c
>> +++ b/libavcodec/hevc_ps.c
>> @@ -628,8 +628,17 @@ 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);
>> +        unsigned horiz_mult, vert_mult;
>> +        if (sps->chroma_format_idc == 1) {
>> +            horiz_mult = 2;
>> +            vert_mult = 2;
>> +        } else if (sps->chroma_format_idc == 2) {
>> +            horiz_mult = 2;
>> +            vert_mult = 1;
>> +        } else {
>> +            horiz_mult = 1;
>> +            vert_mult = 1;
> Can you explain what this patch is meant to do? For chroma_format_idc ==
> 1, 2 and 3 nothing changes with this, only chroma_format_idc == 0, where
> both horiz_mult and vert_mult become 1 instead of 2. Is that
> correct/intended?

Yes, this is intended. See ISO/IEC 23008-2 Table 6-1.

./ffprobe ~/Videos/hevc-monochrome.ts
     Stream #0:0[0x100]: Video: hevc (Rext) ([36][0][0][0] / 0x0024), 
gray(tv), 384x224, 15 fps, 15 tbr, 90k tbn, 15 tbc

The correct size is 384x240.

>> +        }
>>           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 +932,17 @@ 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);
>> +        unsigned horiz_mult, vert_mult;
>> +        if (sps->chroma_format_idc == 1) {
>> +            horiz_mult = 2;
>> +            vert_mult = 2;
>> +        } else if (sps->chroma_format_idc == 2) {
>> +            horiz_mult = 2;
>> +            vert_mult = 1;
>> +        } else {
>> +            horiz_mult = 1;
>> +            vert_mult = 1;
>> +        }
>>           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;
>>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

James Almer Aug. 16, 2018, 2:23 p.m. UTC | #3

On 8/16/2018 2:00 AM, Zhao Zhili wrote:
> 
> 
> On 2018年08月16日 11:46, James Almer wrote:
>> On 8/16/2018 12:29 AM, Zhao Zhili wrote:
>>> ---
>>>   libavcodec/hevc_ps.c | 26 ++++++++++++++++++++++----
>>>   1 file changed, 22 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/libavcodec/hevc_ps.c b/libavcodec/hevc_ps.c
>>> index fbd9fbf..4161ab6 100644
>>> --- a/libavcodec/hevc_ps.c
>>> +++ b/libavcodec/hevc_ps.c
>>> @@ -628,8 +628,17 @@ 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);
>>> +        unsigned horiz_mult, vert_mult;
>>> +        if (sps->chroma_format_idc == 1) {
>>> +            horiz_mult = 2;
>>> +            vert_mult = 2;
>>> +        } else if (sps->chroma_format_idc == 2) {
>>> +            horiz_mult = 2;
>>> +            vert_mult = 1;
>>> +        } else {
>>> +            horiz_mult = 1;
>>> +            vert_mult = 1;
>> Can you explain what this patch is meant to do? For chroma_format_idc ==
>> 1, 2 and 3 nothing changes with this, only chroma_format_idc == 0, where
>> both horiz_mult and vert_mult become 1 instead of 2. Is that
>> correct/intended?
> 
> Yes, this is intended. See ISO/IEC 23008-2 Table 6-1.
> 
> ./ffprobe ~/Videos/hevc-monochrome.ts
>     Stream #0:0[0x100]: Video: hevc (Rext) ([36][0][0][0] / 0x0024),
> gray(tv), 384x224, 15 fps, 15 tbr, 90k tbn, 15 tbc
> 
> The correct size is 384x240.

Alright, can you mention that in the commit message? Thanks!

> 
>>> +        }
>>>           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 +932,17 @@ 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);
>>> +        unsigned horiz_mult, vert_mult;
>>> +        if (sps->chroma_format_idc == 1) {
>>> +            horiz_mult = 2;
>>> +            vert_mult = 2;
>>> +        } else if (sps->chroma_format_idc == 2) {
>>> +            horiz_mult = 2;
>>> +            vert_mult = 1;
>>> +        } else {
>>> +            horiz_mult = 1;
>>> +            vert_mult = 1;
>>> +        }
>>>           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;
>>>
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> 
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Michael Niedermayer Aug. 17, 2018, 12:23 a.m. UTC | #4

On Thu, Aug 16, 2018 at 11:29:09AM +0800, Zhao Zhili wrote:
> ---
>  libavcodec/hevc_ps.c | 26 ++++++++++++++++++++++----
>  1 file changed, 22 insertions(+), 4 deletions(-)
> 
> diff --git a/libavcodec/hevc_ps.c b/libavcodec/hevc_ps.c
> index fbd9fbf..4161ab6 100644
> --- a/libavcodec/hevc_ps.c
> +++ b/libavcodec/hevc_ps.c
> @@ -628,8 +628,17 @@ 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);
> +        unsigned horiz_mult, vert_mult;
> +        if (sps->chroma_format_idc == 1) {
> +            horiz_mult = 2;
> +            vert_mult = 2;
> +        } else if (sps->chroma_format_idc == 2) {
> +            horiz_mult = 2;
> +            vert_mult = 1;
> +        } else {
> +            horiz_mult = 1;
> +            vert_mult = 1;
> +        }
>          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 +932,17 @@ 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);
> +        unsigned horiz_mult, vert_mult;
> +        if (sps->chroma_format_idc == 1) {
> +            horiz_mult = 2;
> +            vert_mult = 2;
> +        } else if (sps->chroma_format_idc == 2) {
> +            horiz_mult = 2;
> +            vert_mult = 1;
> +        } else {
> +            horiz_mult = 1;
> +            vert_mult = 1;
> +        }

this should either stay simple code or it should be factored out.
duplicating this is not ideal.


[...]

diff --git a/libavcodec/hevc_ps.c b/libavcodec/hevc_ps.c
index fbd9fbf..4161ab6 100644
--- a/libavcodec/hevc_ps.c
+++ b/libavcodec/hevc_ps.c
@@ -628,8 +628,17 @@  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);
+        unsigned horiz_mult, vert_mult;
+        if (sps->chroma_format_idc == 1) {
+            horiz_mult = 2;
+            vert_mult = 2;
+        } else if (sps->chroma_format_idc == 2) {
+            horiz_mult = 2;
+            vert_mult = 1;
+        } else {
+            horiz_mult = 1;
+            vert_mult = 1;
+        }
         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 +932,17 @@  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);
+        unsigned horiz_mult, vert_mult;
+        if (sps->chroma_format_idc == 1) {
+            horiz_mult = 2;
+            vert_mult = 2;
+        } else if (sps->chroma_format_idc == 2) {
+            horiz_mult = 2;
+            vert_mult = 1;
+        } else {
+            horiz_mult = 1;
+            vert_mult = 1;
+        }
         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;

[FFmpeg-devel] lavc/hevc_ps: fix crop info

Commit Message

Comments

Patch