* Re: [FFmpeg-devel] [PATCH 1/1] Add vpx range encoder support
2025-03-05 16:29 ` [FFmpeg-devel] [PATCH 1/1] Add vpx " MihirGore
@ 2025-03-05 16:51 ` Andreas Rheinhardt
2025-03-06 22:58 ` Peter Ross
1 sibling, 0 replies; 4+ messages in thread
From: Andreas Rheinhardt @ 2025-03-05 16:51 UTC (permalink / raw)
To: ffmpeg-devel
MihirGore:
> From: MihirGore23 <mihirvgore@outlook.com>
>
> ---
> libavcodec/vpx_rac.h | 75 ++++++++++++++++++++++++++++++++++----------
> 1 file changed, 59 insertions(+), 16 deletions(-)
>
> diff --git a/libavcodec/vpx_rac.h b/libavcodec/vpx_rac.h
> index b158cc0754..f01358f71f 100644
> --- a/libavcodec/vpx_rac.h
> +++ b/libavcodec/vpx_rac.h
> @@ -20,7 +20,7 @@
>
> /**
> * @file
> - * Common VP5-VP9 range decoder stuff
> + * Common VP5-VP9 range encoder and decoder functions
> */
>
> #ifndef AVCODEC_VPX_RAC_H
> @@ -34,24 +34,25 @@
>
> typedef struct VPXRangeCoder {
> int high;
> - int bits; /* stored negated (i.e. negative "bits" is a positive number of
> - bits left) in order to eliminate a negate in cache refilling */
> + int bits;
> const uint8_t *buffer;
> const uint8_t *end;
> unsigned int code_word;
> int end_reached;
> + uint8_t *output_buffer; // Added for encoding
> + uint8_t *output_end; // Added for encoding
What are the advantages of reusing the same structure etc. in contrast
to using dedicated ones?
> } VPXRangeCoder;
>
> extern const uint8_t ff_vpx_norm_shift[256];
> +
> +/*Decoder Functions */
> +
> int ff_vpx_init_range_decoder(VPXRangeCoder *c, const uint8_t *buf, int buf_size);
>
> -/**
> - * returns 1 if the end of the stream has been reached, 0 otherwise.
> - */
> static av_always_inline int vpx_rac_is_end(VPXRangeCoder *c)
> {
> if (c->end <= c->buffer && c->bits >= 0)
> - c->end_reached ++;
> + c->end_reached++;
> return c->end_reached > 10;
> }
>
> @@ -64,7 +65,7 @@ static av_always_inline unsigned int vpx_rac_renorm(VPXRangeCoder *c)
> c->high <<= shift;
> code_word <<= shift;
> bits += shift;
> - if(bits >= 0 && c->buffer < c->end) {
> + if (bits >= 0 && c->buffer < c->end) {
Cosmetics and functional changes should not be mixed.
> code_word |= bytestream_get_be16(&c->buffer) << bits;
> bits -= 16;
> }
> @@ -72,12 +73,6 @@ static av_always_inline unsigned int vpx_rac_renorm(VPXRangeCoder *c)
> return code_word;
> }
>
> -#if ARCH_ARM
> -#include "arm/vpx_arith.h"
> -#elif ARCH_X86
> -#include "x86/vpx_arith.h"
> -#endif
> -
Why are you doing this?
> #ifndef vpx_rac_get_prob
> #define vpx_rac_get_prob vpx_rac_get_prob
> static av_always_inline int vpx_rac_get_prob(VPXRangeCoder *c, uint8_t prob)
> @@ -95,7 +90,6 @@ static av_always_inline int vpx_rac_get_prob(VPXRangeCoder *c, uint8_t prob)
> #endif
>
> #ifndef vpx_rac_get_prob_branchy
> -// branchy variant, to be used where there's a branch based on the bit decoded
> static av_always_inline int vpx_rac_get_prob_branchy(VPXRangeCoder *c, int prob)
> {
> unsigned long code_word = vpx_rac_renorm(c);
> @@ -117,7 +111,6 @@ static av_always_inline int vpx_rac_get_prob_branchy(VPXRangeCoder *c, int prob)
> static av_always_inline int vpx_rac_get(VPXRangeCoder *c)
> {
> unsigned int code_word = vpx_rac_renorm(c);
> - /* equiprobable */
> int low = (c->high + 1) >> 1;
> unsigned int low_shift = low << 16;
> int bit = code_word >= low_shift;
> @@ -132,4 +125,54 @@ static av_always_inline int vpx_rac_get(VPXRangeCoder *c)
> return bit;
> }
>
> +//Encoder Functions
> +
> +int ff_vpx_init_range_encoder(VPXRangeCoder *c, uint8_t *buf, int buf_size);
> +
> +static av_always_inline void vpx_rac_renorm_encoder(VPXRangeCoder *c)
> +{
> + while (c->high < 0x10000) {
> + if (c->output_buffer < c->output_end) {
> + *c->output_buffer++ = c->code_word >> 24;
> + }
> + c->code_word <<= 8;
> + c->high <<= 8;
> + }
> +}
> +
> +static av_always_inline void vpx_rac_put_prob(VPXRangeCoder *c, int bit, uint8_t prob)
> +{
> + unsigned int low = 1 + (((c->high - 1) * prob) >> 8);
> + if (bit) {
> + c->code_word += low << 16;
> + c->high -= low;
> + } else {
> + c->high = low;
> + }
> + vpx_rac_renorm_encoder(c);
> +}
> +
> +static av_always_inline void vpx_rac_put(VPXRangeCoder *c, int bit)
> +{
> + unsigned int low = (c->high + 1) >> 1;
> + if (bit) {
> + c->code_word += low << 16;
> + c->high -= low;
> + } else {
> + c->high = low;
> + }
> + vpx_rac_renorm_encoder(c);
> +}
> +
> +static av_always_inline void vpx_rac_flush(VPXRangeCoder *c)
> +{
> + for (int i = 0; i < 4; i++) {
> + if (c->output_buffer < c->output_end) {
> + *c->output_buffer++ = c->code_word >> 24;
> + }
> + c->code_word <<= 8;
> + }
> +}
> +
> #endif /* AVCODEC_VPX_RAC_H */
> +
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [FFmpeg-devel] [PATCH 1/1] Add vpx range encoder support
2025-03-05 16:29 ` [FFmpeg-devel] [PATCH 1/1] Add vpx " MihirGore
2025-03-05 16:51 ` Andreas Rheinhardt
@ 2025-03-06 22:58 ` Peter Ross
1 sibling, 0 replies; 4+ messages in thread
From: Peter Ross @ 2025-03-06 22:58 UTC (permalink / raw)
To: FFmpeg development discussions and patches
[-- Attachment #1.1: Type: text/plain, Size: 4592 bytes --]
On Wed, Mar 05, 2025 at 09:59:34PM +0530, MihirGore wrote:
> From: MihirGore23 <mihirvgore@outlook.com>
>
Hi,
comments below.
> ---
> libavcodec/vpx_rac.h | 75 ++++++++++++++++++++++++++++++++++----------
> 1 file changed, 59 insertions(+), 16 deletions(-)
>
> diff --git a/libavcodec/vpx_rac.h b/libavcodec/vpx_rac.h
> index b158cc0754..f01358f71f 100644
> --- a/libavcodec/vpx_rac.h
> +++ b/libavcodec/vpx_rac.h
> @@ -20,7 +20,7 @@
>
> /**
> * @file
> - * Common VP5-VP9 range decoder stuff
> + * Common VP5-VP9 range encoder and decoder functions
> */
>
> #ifndef AVCODEC_VPX_RAC_H
> @@ -34,24 +34,25 @@
>
> typedef struct VPXRangeCoder {
> int high;
> - int bits; /* stored negated (i.e. negative "bits" is a positive number of
> - bits left) in order to eliminate a negate in cache refilling */
> + int bits;
^^^^^
there is some some unnecessary whitespace after 'bits;'
also why did you delete the original comment?
> const uint8_t *buffer;
> const uint8_t *end;
> unsigned int code_word;
> int end_reached;
> + uint8_t *output_buffer; // Added for encoding
> + uint8_t *output_end; // Added for encoding
> } VPXRangeCoder;
>
> extern const uint8_t ff_vpx_norm_shift[256];
> +
> +/*Decoder Functions */
> +
> int ff_vpx_init_range_decoder(VPXRangeCoder *c, const uint8_t *buf, int buf_size);
>
> -/**
> - * returns 1 if the end of the stream has been reached, 0 otherwise.
> - */
> static av_always_inline int vpx_rac_is_end(VPXRangeCoder *c)
> {
> if (c->end <= c->buffer && c->bits >= 0)
> - c->end_reached ++;
> + c->end_reached++;
> return c->end_reached > 10;
> }
>
> @@ -64,7 +65,7 @@ static av_always_inline unsigned int vpx_rac_renorm(VPXRangeCoder *c)
> c->high <<= shift;
> code_word <<= shift;
> bits += shift;
> - if(bits >= 0 && c->buffer < c->end) {
> + if (bits >= 0 && c->buffer < c->end) {
> code_word |= bytestream_get_be16(&c->buffer) << bits;
> bits -= 16;
> }
> @@ -72,12 +73,6 @@ static av_always_inline unsigned int vpx_rac_renorm(VPXRangeCoder *c)
> return code_word;
> }
>
> -#if ARCH_ARM
> -#include "arm/vpx_arith.h"
> -#elif ARCH_X86
> -#include "x86/vpx_arith.h"
> -#endif
> -
> #ifndef vpx_rac_get_prob
> #define vpx_rac_get_prob vpx_rac_get_prob
> static av_always_inline int vpx_rac_get_prob(VPXRangeCoder *c, uint8_t prob)
> @@ -95,7 +90,6 @@ static av_always_inline int vpx_rac_get_prob(VPXRangeCoder *c, uint8_t prob)
> #endif
>
> #ifndef vpx_rac_get_prob_branchy
> -// branchy variant, to be used where there's a branch based on the bit decoded
> static av_always_inline int vpx_rac_get_prob_branchy(VPXRangeCoder *c, int prob)
> {
> unsigned long code_word = vpx_rac_renorm(c);
> @@ -117,7 +111,6 @@ static av_always_inline int vpx_rac_get_prob_branchy(VPXRangeCoder *c, int prob)
> static av_always_inline int vpx_rac_get(VPXRangeCoder *c)
> {
> unsigned int code_word = vpx_rac_renorm(c);
> - /* equiprobable */
> int low = (c->high + 1) >> 1;
> unsigned int low_shift = low << 16;
> int bit = code_word >= low_shift;
> @@ -132,4 +125,54 @@ static av_always_inline int vpx_rac_get(VPXRangeCoder *c)
> return bit;
> }
>
> +//Encoder Functions
> +
> +int ff_vpx_init_range_encoder(VPXRangeCoder *c, uint8_t *buf, int buf_size);
> +
ff_vpx_init_range_encoder function is not defined anywhere.
did you forget to commit vpx_rac.c changes?
> +static av_always_inline void vpx_rac_flush(VPXRangeCoder *c)
> +{
> + for (int i = 0; i < 4; i++) {
> + if (c->output_buffer < c->output_end) {
> + *c->output_buffer++ = c->code_word >> 24;
> + }
> + c->code_word <<= 8;
> + }
> +}
after calling vpx_rac_flush() how does the caller determine the final size
of the output buffer?
finally, does the output of your encoder work with the existing decoder?
simple test case:
```
VPXRangeCoder e;
uint8_t buf[1024] = {0};
ff_vpx_init_range_encoder(&e, buf, sizeof(buf));
vpx_rac_put(&e, 1);
vpx_rac_put(&e, 0);
vpx_rac_put(&e, 1);
vpx_rac_flush(&e);
VPXRangeCoder d;
ff_vpx_init_range_decoder(&d, buf, e.output_buffer - buf);
printf("%d\n", vpx_rac_get(&d));
printf("%d\n", vpx_rac_get(&d));
printf("%d\n", vpx_rac_get(&d));
```
-- Peter
(A907 E02F A6E5 0CD2 34CD 20D2 6760 79C5 AC40 DD6B)
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]
[-- Attachment #2: Type: text/plain, Size: 251 bytes --]
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
^ permalink raw reply [flat|nested] 4+ messages in thread