Re: [FFmpeg-devel] [PATCH 2/3 v3] avcodec/cbs_h265: move the payload_extension_present check into its own function
On 4/30/2020 3:08 PM, Andreas Rheinhardt wrote:
> James Almer:
>> Will be reused in the following patch.
>>
>> Signed-off-by: James Almer
>> ---
>> Moved the comment for the function to its new place, but otherwise, no
>> difference compared to v2.
>>
>> libavcodec/cbs_h2645.c| 10 ++
>> libavcodec/cbs_h265_syntax_template.c | 9 +++--
>> 2 files changed, 13 insertions(+), 6 deletions(-)
>>
>> diff --git a/libavcodec/cbs_h2645.c b/libavcodec/cbs_h2645.c
>> index d42073cc5a..095e449ddc 100644
>> --- a/libavcodec/cbs_h2645.c
>> +++ b/libavcodec/cbs_h2645.c
>> @@ -233,6 +233,16 @@ static int cbs_write_se_golomb(CodedBitstreamContext
>> *ctx, PutBitContext *pbc,
>> return 0;
>> }
>>
>> +// payload_extension_present() - true if we are before the last 1-bit
>> +// in the payload structure, which must be in the last byte.
>> +static int cbs_h265_payload_extension_present(GetBitContext *gbc, uint32_t
>> payload_size,
>> + int cur_pos)
>> +{
>> +int bits_left = payload_size * 8 - cur_pos;
>> +return (bits_left > 0 &&
>> +(bits_left > 7 || show_bits(gbc, bits_left) &
>> MAX_UINT_BITS(bits_left - 1)));
> Consider a buffering period SEI message whose bit position is 7 mod 8
> before the payload extension present check and where there is only one
> bit, namely a zero bit left in the SEI message. According to the
> definition of payload_extension_present() in the standard, it will
> Consider a buffering period SEI message whose bit position is 7 mod 8
> before the payload extension present check and where there is only one
> bit, namely a zero bit left in the SEI message. According to the
> definition of payload_extension_present() in the standard, it will
> return true and the last bit is use_alt_cpb_params_flag (with a value
> zero); yet nevertheless the more_data_in_payload() check after the
> buffering_period() returns false. This is a valid SEI message according
> to the current version of the standard (in fact, for all versions except
> for the very first version).
I consider a Buffering Period SEI message as you described it as being
invalid precisely because it wont work on all revisions of the spec. The
hypotetical encoder simply did not write the SEI message with backwards
compatibility in mind, which would have been trivial to avoid.
>
> Yet the check above will return false in this scenario and the SEI
> message unit will be considered invalid. I don't object to this, but you
> should mention this discrepancy in a comment.
>
> - Andreas
>
>> +}
>> +
>> #define HEADER(name) do { \
>> ff_cbs_trace_header(ctx, name); \
>> } while (0)
>> diff --git a/libavcodec/cbs_h265_syntax_template.c
>> b/libavcodec/cbs_h265_syntax_template.c
>> index a51b12cfc6..ed08b06e9c 100644
>> --- a/libavcodec/cbs_h265_syntax_template.c
>> +++ b/libavcodec/cbs_h265_syntax_template.c
>> @@ -1572,7 +1572,7 @@ static int
>> FUNC(sei_buffering_period)(CodedBitstreamContext *ctx, RWContext *rw,
>> int err, i, length;
>>
>> #ifdef READ
>> -int start_pos, end_pos, bits_left;
>> +int start_pos, end_pos;
>> start_pos = get_bits_count(rw);
>> #endif
>>
>> @@ -1651,12 +1651,9 @@ static int
>> FUNC(sei_buffering_period)(CodedBitstreamContext *ctx, RWContext *rw,
>> }
>>
>> #ifdef READ
>> -// payload_extension_present() - true if we are before the last 1-bit
>> -// in the payload structure, which must be in the last byte.
>> end_pos = get_bits_count(rw);
>> -bits_left = *payload_size * 8 - (end_pos - start_pos);
>> -if (bits_left > 0 &&
>> -(bits_left > 7 || ff_ctz(show_bits(rw, bits_left)) < bits_left - 1))
>> +if (cbs_h265_payload_extension_present(rw, *payload_size,
>> + end_pos - start_pos))
>> flag(use_alt_cpb_params_flag);
>> else
>> infer(use_alt_cpb_params_flag, 0);
>>
>
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
>
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 2/3 v3] avcodec/cbs_h265: move the payload_extension_present check into its own function
James Almer:
> Will be reused in the following patch.
>
> Signed-off-by: James Almer
> ---
> Moved the comment for the function to its new place, but otherwise, no
> difference compared to v2.
>
> libavcodec/cbs_h2645.c| 10 ++
> libavcodec/cbs_h265_syntax_template.c | 9 +++--
> 2 files changed, 13 insertions(+), 6 deletions(-)
>
> diff --git a/libavcodec/cbs_h2645.c b/libavcodec/cbs_h2645.c
> index d42073cc5a..095e449ddc 100644
> --- a/libavcodec/cbs_h2645.c
> +++ b/libavcodec/cbs_h2645.c
> @@ -233,6 +233,16 @@ static int cbs_write_se_golomb(CodedBitstreamContext
> *ctx, PutBitContext *pbc,
> return 0;
> }
>
> +// payload_extension_present() - true if we are before the last 1-bit
> +// in the payload structure, which must be in the last byte.
> +static int cbs_h265_payload_extension_present(GetBitContext *gbc, uint32_t
> payload_size,
> + int cur_pos)
> +{
> +int bits_left = payload_size * 8 - cur_pos;
> +return (bits_left > 0 &&
> +(bits_left > 7 || show_bits(gbc, bits_left) &
> MAX_UINT_BITS(bits_left - 1)));
Consider a buffering period SEI message whose bit position is 7 mod 8
before the payload extension present check and where there is only one
bit, namely a zero bit left in the SEI message. According to the
definition of payload_extension_present() in the standard, it will
Consider a buffering period SEI message whose bit position is 7 mod 8
before the payload extension present check and where there is only one
bit, namely a zero bit left in the SEI message. According to the
definition of payload_extension_present() in the standard, it will
return true and the last bit is use_alt_cpb_params_flag (with a value
zero); yet nevertheless the more_data_in_payload() check after the
buffering_period() returns false. This is a valid SEI message according
to the current version of the standard (in fact, for all versions except
for the very first version).
Yet the check above will return false in this scenario and the SEI
message unit will be considered invalid. I don't object to this, but you
should mention this discrepancy in a comment.
- Andreas
> +}
> +
> #define HEADER(name) do { \
> ff_cbs_trace_header(ctx, name); \
> } while (0)
> diff --git a/libavcodec/cbs_h265_syntax_template.c
> b/libavcodec/cbs_h265_syntax_template.c
> index a51b12cfc6..ed08b06e9c 100644
> --- a/libavcodec/cbs_h265_syntax_template.c
> +++ b/libavcodec/cbs_h265_syntax_template.c
> @@ -1572,7 +1572,7 @@ static int
> FUNC(sei_buffering_period)(CodedBitstreamContext *ctx, RWContext *rw,
> int err, i, length;
>
> #ifdef READ
> -int start_pos, end_pos, bits_left;
> +int start_pos, end_pos;
> start_pos = get_bits_count(rw);
> #endif
>
> @@ -1651,12 +1651,9 @@ static int
> FUNC(sei_buffering_period)(CodedBitstreamContext *ctx, RWContext *rw,
> }
>
> #ifdef READ
> -// payload_extension_present() - true if we are before the last 1-bit
> -// in the payload structure, which must be in the last byte.
> end_pos = get_bits_count(rw);
> -bits_left = *payload_size * 8 - (end_pos - start_pos);
> -if (bits_left > 0 &&
> -(bits_left > 7 || ff_ctz(show_bits(rw, bits_left)) < bits_left - 1))
> +if (cbs_h265_payload_extension_present(rw, *payload_size,
> + end_pos - start_pos))
> flag(use_alt_cpb_params_flag);
> else
> infer(use_alt_cpb_params_flag, 0);
>
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
[FFmpeg-devel] [PATCH 2/3 v3] avcodec/cbs_h265: move the payload_extension_present check into its own function
Will be reused in the following patch.
Signed-off-by: James Almer
---
Moved the comment for the function to its new place, but otherwise, no
difference compared to v2.
libavcodec/cbs_h2645.c| 10 ++
libavcodec/cbs_h265_syntax_template.c | 9 +++--
2 files changed, 13 insertions(+), 6 deletions(-)
diff --git a/libavcodec/cbs_h2645.c b/libavcodec/cbs_h2645.c
index d42073cc5a..095e449ddc 100644
--- a/libavcodec/cbs_h2645.c
+++ b/libavcodec/cbs_h2645.c
@@ -233,6 +233,16 @@ static int cbs_write_se_golomb(CodedBitstreamContext *ctx,
PutBitContext *pbc,
return 0;
}
+// payload_extension_present() - true if we are before the last 1-bit
+// in the payload structure, which must be in the last byte.
+static int cbs_h265_payload_extension_present(GetBitContext *gbc, uint32_t
payload_size,
+ int cur_pos)
+{
+int bits_left = payload_size * 8 - cur_pos;
+return (bits_left > 0 &&
+(bits_left > 7 || show_bits(gbc, bits_left) &
MAX_UINT_BITS(bits_left - 1)));
+}
+
#define HEADER(name) do { \
ff_cbs_trace_header(ctx, name); \
} while (0)
diff --git a/libavcodec/cbs_h265_syntax_template.c
b/libavcodec/cbs_h265_syntax_template.c
index a51b12cfc6..ed08b06e9c 100644
--- a/libavcodec/cbs_h265_syntax_template.c
+++ b/libavcodec/cbs_h265_syntax_template.c
@@ -1572,7 +1572,7 @@ static int
FUNC(sei_buffering_period)(CodedBitstreamContext *ctx, RWContext *rw,
int err, i, length;
#ifdef READ
-int start_pos, end_pos, bits_left;
+int start_pos, end_pos;
start_pos = get_bits_count(rw);
#endif
@@ -1651,12 +1651,9 @@ static int
FUNC(sei_buffering_period)(CodedBitstreamContext *ctx, RWContext *rw,
}
#ifdef READ
-// payload_extension_present() - true if we are before the last 1-bit
-// in the payload structure, which must be in the last byte.
end_pos = get_bits_count(rw);
-bits_left = *payload_size * 8 - (end_pos - start_pos);
-if (bits_left > 0 &&
-(bits_left > 7 || ff_ctz(show_bits(rw, bits_left)) < bits_left - 1))
+if (cbs_h265_payload_extension_present(rw, *payload_size,
+ end_pos - start_pos))
flag(use_alt_cpb_params_flag);
else
infer(use_alt_cpb_params_flag, 0);
--
2.26.2
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
