Add frame level on/off bawp by jianle973 · Pull Request #5156 · AOMediaCodec/avm

jianle973 · 2026-07-24T22:16:24Z

Enable frame level bawp based on histogram.

33 frame RA results (speed 1) on v15: +0.01% bit rate with 98.5% encoding time.

+---------+--------+-------+-------+-------+----------+----------+
| Summary | Y | U | V | YUV | Enc-time | Dec-time |
+---------+--------+-------+-------+-------+----------+----------+
| A1 | +0.01% | +0.02% | +0.02% | +0.02% | 99.42% | 100.70% |
| A2 | +0.01% | -0.02% | +0.13% | +0.02% | 99.34% | 99.96% |

+---------+--------+-------+-------+-------+----------+----------+

enable frame level bwap based on histogram

yeqing-wu · 2026-07-24T23:50:37Z


+#if CONFIG_FAST_BAWP
+  // Indicates whenter frame level on/off fast bawp is used.
+  int enable_fast_bawp;


Same comment: It is better to add enable_fast_bawp as a new speed feature, instead of setting it in oxcf.

Done, thanks!

yeqing-wu · 2026-07-25T00:29:55Z

+  int numValues = 1 << cm->seq_params.bit_depth;
+
+  int32_t *curr_hist = (int32_t *)avm_calloc(numValues, sizeof(int32_t));
+  int32_t *ref_hist = (int32_t *)avm_calloc(numValues, sizeof(int32_t));


Should we check whether the malloc succeeded, using CHECK_MEM_ERROR?

Also, even though this is a frame-level allocation/free, should we instead allocate and free it when the encoder is created and released? That would avoid the alloc/free on every frame and every pass.

Added CHECK_MEM_ERROR.

Since the memory size is small, I still keep it as frame level operation.

yeqing-wu · 2026-07-25T00:31:21Z

 }

+#if CONFIG_FAST_BAWP
+void get_histogram(int32_t *hist, const YV12_BUFFER_CONFIG *buf, int width,


Should we declare it as static if it's only used in this source file?

Yes, static is added.

yeqing-wu · 2026-07-25T00:36:27Z

+        }
+
+        if (diffHist > hist_diff_thres) {
+          enable_curr_pic_bawp = 1;


Once if (diffHist > hist_diff_thres) is satisfied, should we return here directly, to save the cost of the further histogram computation and checks? Based on the logic, in any case where diffHist > hist_diff_thres, enable_curr_pic_bawp would always be true, right? If yes, then we do not need enable_curr_pic_bawp

Yes, we should return immediately once (diffHist > hist_diff_thres) meets. In the code, this logic is fulfilled by

if (diff_hist > hist_diff_thres) { enable_curr_pic_bawp = 1; **break;** }

yeqing-wu · 2026-07-25T00:39:56Z

+    const RefCntBuffer *ref_buf = get_ref_frame_buf(cm, rf);
+    if (ref_buf != NULL) {
+      int ref_width = ref_buf->mi_cols * 4;
+      int ref_height = ref_buf->mi_rows * 4;


For resolutions whose dimensions aren't multiples of 4, would the pixel values outside the frame (I can't recall whether the area outside the frame is padded or not) producing a wrong histogram and leading to a wrong decision?

In my understanding, the pixels outside the input frames boundary are padded, and the reference also stored in the padded size. The padded areas also need to be encoded. It might be more accurate to also count them.

yeqing-wu · 2026-07-25T00:41:24Z

+
+  int width = source->y_width;
+  int height = source->y_height;
+  int num_samples = width * height;


Use const if the variable's value won't be changed. Same comment for other variables inside this function.

yeqing-wu · 2026-07-25T00:43:36Z

+      int ref_height = ref_buf->mi_rows * 4;
+
+      if (width == ref_width && height == ref_height) {
+        memset(ref_hist, 0, sizeof(int32_t) * numValues);


Let's use the AVM coding style for numValues and diffHist.

yeqing-wu · 2026-07-25T00:45:24Z

+  }
+}
+
+int av2_set_on_bawp_picture_level(AV2_COMP *cpi) {


Change it from AV2_COMP *cpi to const AV2_COMP *const cpi, since we don't need to modify its members?

jianle973 · 2026-07-25T06:17:29Z

Thanks @yeqing-wu for reviewing the PR. I have tried to address all the comments in the new commit.

yeqing-wu

LGTM.

It is required to provide the anchor commit number and the A1/A2 results in the commit message. I think you can put this info in the PR conversation message. To prevent GitHub from messing up your table, you can wrap it with ``` before and after, like:

+---------+--------+-------+-------+-------+----------+----------+
| Summary |   Y    |   U   |   V   |  YUV  | Enc-time | Dec-time |
+---------+--------+-------+-------+-------+----------+----------+
| A1      | -0.02% | 0.24% | 0.21% | 0.00% | 97.8%    | 100.5%   |
| A2      | 0.10%  | 0.15% | 0.03% | 0.09% | 95.8%    | 99.8%    |
+---------+--------+-------+-------+-------+----------+----------+

leolzhao · 2026-07-27T17:19:12Z

Please add the separate class A1 and A2 results in the MR message? @jianle973

jianle973 · 2026-07-27T17:59:28Z

33 frames results (speed 1) as follows:

+---------+--------+-------+-------+-------+----------+----------+
| Summary | Y | U | V | YUV | Enc-time | Dec-time |
+---------+--------+-------+-------+-------+----------+----------+
| A1 | +0.01% | +0.02% | +0.02% | +0.02% | 99.42% | 100.70% |
| A2 | +0.01% | -0.02% | +0.13% | +0.02% | 99.34% | 99.96% |

+---------+--------+-------+-------+-------+----------+----------+

yeqing-wu · 2026-07-27T18:10:00Z

Since it doesn’t meet the acceptance criteria for speed 1, should we move it to speed 4? And what is the commit ID of the anchor?

leolzhao · 2026-07-27T18:12:31Z

Since it doesn’t meet the acceptance criteria for speed 1, should we move it to speed 4?

Yes, we can enable it for speed 4 if the resutls with cpu-used = 4 is similar or better.

jianle973 · 2026-07-27T18:49:03Z

Since it doesn’t meet the acceptance criteria for speed 1, should we move it to speed 4?

Yes, we can enable it for speed 4 if the resutls with cpu-used = 4 is similar or better.

It was 98.5% encoding time when I tested it on top v15. Not sure what's the problem. The running time deviation on our cluster could be the reason.

Add frame level on/off bawp

9763f48

enable frame level bwap based on histogram

jianle973 requested review from leolzhao, yeqing-wu and yunqingwang1 July 24, 2026 22:16

aom-bot added cla-signed labels Jul 24, 2026

yeqing-wu reviewed Jul 25, 2026

View reviewed changes

Address Yeqing's review

9a7e4ae

yeqing-wu approved these changes Jul 25, 2026

View reviewed changes

yunqingwang1 approved these changes Jul 27, 2026

View reviewed changes

Uh oh!

Conversation

jianle973 commented Jul 24, 2026 • edited by leolzhao Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jianle973 Jul 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jianle973 commented Jul 25, 2026

Uh oh!

yeqing-wu left a comment

Choose a reason for hiding this comment

Uh oh!

leolzhao commented Jul 27, 2026

Uh oh!

jianle973 commented Jul 27, 2026

Uh oh!

yeqing-wu commented Jul 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

leolzhao commented Jul 27, 2026

Uh oh!

jianle973 commented Jul 27, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jianle973 commented Jul 24, 2026 •

edited by leolzhao

Loading

jianle973 Jul 25, 2026 •

edited

Loading

yeqing-wu commented Jul 27, 2026 •

edited

Loading