Opened 6 years ago

Closed 6 years ago

#3204 closed defect (fixed)

VC1 Intensity Compensation is always used once encountered

Reported by: mcarter Owned by:
Priority: important Component: avcodec
Version: git-master Keywords: vc1 regression
Cc: Blocked By:
Blocking: Reproduced by developer: yes
Analyzed by developer: no

Description

I noticed that VC1 decoding performance dropped after upgrading to the latest git code. I profiled it and found that a lot (double-digit %) of time was spent in libavcodec/vc1dec.c:vc1_mc_1mv under "if (use_ic) ...".

I created the attached patch, intcomp.patch (just output from git diff), to instrument libavcodec and I ran:

% ffmpeg -t 60 -i BigBuckBunny_1080p24.wmv -an -f null -
...
g_identity_lut_count: 41704276, g_non_identity_lut_count: 154612
g_vc1_mc_1mv_calls: 10474337, g_use_ic_times: 10464722

The file is from http://bit.ly/bbbwmv which is from http://msdn.microsoft.com/en-us/expression/hh553539.aspx.

The result was that for 60 sec of decoding this VC1 Advanced Profile video, vc1_mc_1mv was called 10,474,337 times and of that, it did intensity compensation 10,464,722 times (99.9% of the time). It seems highly improbable that intensity compensation is needed on every single call. Indeed, the instrumentation shows the number of times the intensity compensation lookup tables (luty, lutuv) don't do anything (because they are the identity mapping) and it is the majority of the time (41704276/(41704276+154612)=99.63%). Thus, this suggests a bug in the code.

I looked at the git history and this might be the commit that introduced this issue, but I'm not sure:

http://git.videolan.org/?p=ffmpeg.git;a=commitdiff;h=782ebd611824c95201123aeccc85809ce1554e6e

I looked at the source code and it seems that once v->next_use_ic and/or v->last_use_ic are set to 1, they might never go back to 0, perhaps causing the issue (once intensity compensation is encountered in the file, it is used forever). But I'm not very familiar with this code, so I'm not sure. Maybe I'm totally misunderstanding everything and intensity compensation is required for the entire file, even though most of the times it doesn't do anything?

Attachments (1)

intcomp.patch.txt (2.7 KB) - added by mcarter 6 years ago.
vc1dec.c instrumentation patch

Download all attachments as: .zip

Change History (5)

Changed 6 years ago by mcarter

vc1dec.c instrumentation patch

comment:1 Changed 6 years ago by cehoyos

  • Keywords regression added; intensity compensation removed
  • Priority changed from normal to important
  • Reproduced by developer set
  • Status changed from new to open

782ebd61 is the cause for a >5% speed regression in vc1 decoding.

comment:2 Changed 6 years ago by mcarter

Here is a (probably suboptimal) naive patch that might solve the issue:

In vc1.c, in rotate_luts() after v->curr_use_ic = 0, apply this patch:

ROTATE(int tmp,             v->last_use_ic, v->next_use_ic, v->curr_use_ic, v->aux_use_ic);
ROTATE(uint8_t tmp[2][256], v->last_luty,   v->next_luty,   v->curr_luty,   v->aux_luty);
ROTATE(uint8_t tmp[2][256], v->last_lutuv,  v->next_lutuv,  v->curr_lutuv,  v->aux_lutuv);

INIT_LUT(32, 0, v->curr_luty[0], v->curr_lutuv[0], 0);
INIT_LUT(32, 0, v->curr_luty[1], v->curr_lutuv[1], 0);
v->curr_use_ic = 0;

+if (v->curr_luty == v->next_luty) {
+    v->next_use_ic = 0;
+}

Here's why this makes sense: the previous lines call INIT_LUT(32, 0, v->curr_luty... which sets v->curr_lut{y,uv}[0,1] to the identity mapping. But note that due to the ROTATE() calls, v->curr_luty points to either v->next_luty or v->aux_luty. If v->curr_luty points to v->next_luty, then the INIT_LUT() calls just set v->next_luty to the identity mapping. If v->next_luty is set to the identity mapping, then v->next_use_ic should be set to 0, just as v->curr_use_ic has already been set to 0. (We don't need to set v->aux_use_ic since it is never read by any code.)

An alternative patch would be to use the same test that ROTATE() uses: if (!(v->s.pict_type == AV_PICTURE_TYPE_BI || v->s.pict_type == AV_PICTURE_TYPE_B)) { v->next_use_ic = 0; }

I'm not 100% sure if either of these are a good fix, but I hope it gives some insight into the problem.

comment:3 Changed 6 years ago by cehoyos

Please send all patches to the ffmpeg-devel mailing list where they can be discussed and will be reviewed.

comment:4 Changed 6 years ago by michael

  • Resolution set to fixed
  • Status changed from open to closed

Patch applied and simplified
sorry for the delay, i have totally missed the patch in the ticket.
patches should be sent to ffmpeg-devel ML
also dont hesitate to mail / ping me per private mail if some change from me caused a regression

Note: See TracTickets for help on using tickets.