Disable two simple motion related features at speed 2

[chengchen-google]

The speed feature's performance at different speeds are shown below: (base commit: b8f0795)

Speed     Set   BD-YUV    EncTime (% Base)    Speedup	Ratio
Speed_2	  A1    -0.22%     106.02% 	        +5.68%	    25.8
Speed_2	  A2    -0.99%     122.19% 	       +18.16%  	18.3
Speed_3	  A1    -0.20%     104.29% 	        +4.11%  	20.6
Speed_3	  A2    -0.18%     107.92% 	        +7.34%  	40.8
Speed_4	  A1    -0.32%     106.05% 	        +5.70%  	17.8
Speed_4	  A2    -0.24%     107.78% 	        +7.22%  	30.1

The breakdown of the two features is: (on an older commit cc53250)

+----------+---------------------------------------------+-----------------------------+------------------------------------+--------------------------------------+
| Test     | Feature Disabled                            | State of Other SMS Feature  | Set A1 (4K, 17f) BD-YUV / EncTime  | Set A2 (1080p, 33f) BD-YUV / EncTime |
+----------+---------------------------------------------+-----------------------------+------------------------------------+--------------------------------------+
| Test A   | simple_motion_search_split OFF              | early_term_none ON          | -0.34% / 102.19%                   | -0.45% / 103.93%                     |
| Test B   | simple_motion_search_early_term_none OFF     | sms_split ON                | -1.22% / 111.83%                   | -0.66% / 115.25%                     |
| Both OFF | Both SMS Features OFF                       | Both OFF                    | -1.58% / 113.85%                   | -1.11% / 119.93%                     |
+----------+---------------------------------------------+-----------------------------+------------------------------------+--------------------------------------+

We turn these features off for speed 2 since they are below the bar.

With this change, we recover -0.22% (A1), -0.99% (A2) coding gains, with 6% (A1), 22% (A2) encoder slowdown.

The current encoder performance before and after the change is:

Configuration	  Set  BD-YUV   EncTime (% Spd 0)
Speed 1 Base	  A1   +1.70%   31.67%
Speed 1 Base	  A2   +2.80%   22.22%
Speed 2 Before	  A1   +5.33%   11.06%
Speed 2 Before	  A2   +5.62%    9.63%
Speed 2 After	  A1   +5.10%   11.73%
Speed 2 After	  A2   +4.57%   11.77%

STATS_CHANGED for speed 2.

[urvangjoshi]

@chengchen-google : two questions:
(1) Have you checked the breakdown of the 2 speed features?
(2) Note that simple_motion_search_early_term_none is still on for some cases (at speed >= 1)

See: https://github.com/AOMediaCodec/avm/blob/av2-enc/av2/encoder/speed_features.c#L1134

  if (cpi->speed >= 1) {
    if (is_720p_or_lesser && !cm->features.allow_screen_content_tools) {
      sf->part_sf.simple_motion_search_early_term_none =
          cm->current_frame.pyramid_level > 4 ? 1 : 0;
    }

It may be worth testing if this is still effective or not.
Either way, will be good to mention this in PR description for future reference.

[chengchen-google]

@chengchen-google : two questions: (1) Have you checked the breakdown of the 2 speed features? (2) Note that simple_motion_search_early_term_none is still on for some cases (at speed >= 1)

See: https://github.com/AOMediaCodec/avm/blob/av2-enc/av2/encoder/speed_features.c#L1134

  if (cpi->speed >= 1) {
    if (is_720p_or_lesser && !cm->features.allow_screen_content_tools) {
      sf->part_sf.simple_motion_search_early_term_none =
          cm->current_frame.pyramid_level > 4 ? 1 : 0;
    }

It may be worth testing if this is still effective or not. Either way, will be good to mention this in PR description for future reference.

(1) Yes. I have tested each speed feature. See the following.

Test	Feature Disabled	State of Other SMS Feature	Set A1 (4K, 17f) BD-YUV / EncTime	Set A2 (1080p, 33f) BD-YUV / EncTime
Test A	simple_motion_search_split OFF	early_term_none ON	-0.34% / 102.19%	-0.45% / 103.93%
Test B	simple_motion_search_early_term_none OFF	sms_split ON	-1.22% / 111.83%	-0.66% / 115.25%
Both OFF	Both SMS Features OFF	Both OFF	-1.58% / 113.85%	-1.11% / 119.93%

None of the two features have good tradeoffs for speed 2.

(2) Yes. The speed feature should still have impact on speed 1. Since speed 1 is now in a good shape, I haven't tested them at speed 1.

I also updated the commit message and description.

[urvangjoshi]

@chengchen-google : two questions: (1) Have you checked the breakdown of the 2 speed features? (2) Note that simple_motion_search_early_term_none is still on for some cases (at speed >= 1)
See: https://github.com/AOMediaCodec/avm/blob/av2-enc/av2/encoder/speed_features.c#L1134

  if (cpi->speed >= 1) {
    if (is_720p_or_lesser && !cm->features.allow_screen_content_tools) {
      sf->part_sf.simple_motion_search_early_term_none =
          cm->current_frame.pyramid_level > 4 ? 1 : 0;
    }

It may be worth testing if this is still effective or not. Either way, will be good to mention this in PR description for future reference.

(1) Yes. I have tested each speed feature. See the following.

Test Feature Disabled State of Other SMS Feature Set A1 (4K, 17f) BD-YUV / EncTime Set A2 (1080p, 33f) BD-YUV / EncTime
Test A simple_motion_search_split OFF early_term_none ON -0.34% / 102.19% -0.45% / 103.93%
Test B simple_motion_search_early_term_none OFF sms_split ON -1.22% / 111.83% -0.66% / 115.25%
Both OFF Both SMS Features OFF Both OFF -1.58% / 113.85% -1.11% / 119.93%
None of the two features have good tradeoffs for speed 2.

(2) Yes. The speed feature should still have impact on speed 1. Since speed 1 is now in a good shape, I haven't tested them at speed 1.

I also updated the commit message and description.

@chengchen-google : I'm confused.
The "Both OFF" numbers in your breakdown table doesn't seem to match the "The speed feature's performance at different speed" table in PR description. Can you explain?

[urvangjoshi]

The av2-enc branch itself was updated (rebased) yesterday. Pls rebase your PR on top of it.

[chengchen-google]

@chengchen-google : two questions: (1) Have you checked the breakdown of the 2 speed features? (2) Note that simple_motion_search_early_term_none is still on for some cases (at speed >= 1)
See: https://github.com/AOMediaCodec/avm/blob/av2-enc/av2/encoder/speed_features.c#L1134

  if (cpi->speed >= 1) {
    if (is_720p_or_lesser && !cm->features.allow_screen_content_tools) {
      sf->part_sf.simple_motion_search_early_term_none =
          cm->current_frame.pyramid_level > 4 ? 1 : 0;
    }

It may be worth testing if this is still effective or not. Either way, will be good to mention this in PR description for future reference.

(1) Yes. I have tested each speed feature. See the following.
Test Feature Disabled State of Other SMS Feature Set A1 (4K, 17f) BD-YUV / EncTime Set A2 (1080p, 33f) BD-YUV / EncTime
Test A simple_motion_search_split OFF early_term_none ON -0.34% / 102.19% -0.45% / 103.93%
Test B simple_motion_search_early_term_none OFF sms_split ON -1.22% / 111.83% -0.66% / 115.25%
Both OFF Both SMS Features OFF Both OFF -1.58% / 113.85% -1.11% / 119.93%
None of the two features have good tradeoffs for speed 2.
(2) Yes. The speed feature should still have impact on speed 1. Since speed 1 is now in a good shape, I haven't tested them at speed 1.
I also updated the commit message and description.

@chengchen-google : I'm confused. The "Both OFF" numbers in your breakdown table doesn't seem to match the "The speed feature's performance at different speed" table in PR description. Can you explain?

Thanks for catching that.
They are tested on different head. The Both OFF tests are back on commit cc53250.
The PR's original description is based on a much more recent commit b8f0795.

Do you want me to run the test on the more recent commit too?

[urvangjoshi]

@chengchen-google : I'm confused. The "Both OFF" numbers in your breakdown table doesn't seem to match the "The speed feature's performance at different speed" table in PR description. Can you explain?

Thanks for catching that. They are tested on different head. The Both OFF tests are back on commit cc53250. The PR's original description is based on a much more recent commit b8f0795.

Do you want me to run the test on the more recent commit too?

Oh OK. No need to test again, as breakdown seems clear.
But pls clarify this in PR description to avoid confusion.

[yunqingwang1]

Question: this change only affect speed 2 (but not speed > 2)?

[chengchen-google]

Question: this change only affect speed 2 (but not speed > 2)?

Yes. It only affects speed 2.

[yeqing-wu]

@chengchen-google,

After the merge, the commit message was messed up. Should we fix it, @urvangjoshi?

a3c9dd2

[yeqing-wu]

I think you can add ``` before and after the table. Then, the Markdown style of the GitHub commit message won’t mess up that.

[chengchen-google]

I think you can add ``` before and after the table. Then, the Markdown style of the GitHub commit message won’t mess up that.

I edit the commit message. Hopefully it looks better now.

[yeqing-wu]

Since this PR has already been merged, how should we proceed to address the issue with the messed commit message? @urvangjoshi, do you have any thoughts on this?

[urvangjoshi]

Since this PR has already been merged, how should we proceed to address the issue with the messed commit message? @urvangjoshi, do you have any thoughts on this?

Yes, the only option to fix commit message is to revert that commit and recreate a new PR with proper commit message.
Probably not worth the hassle.