[Ffmpeg-devel] few remarks for h264 decoder

[Gábor Kovács]

>> 3.
>> pred_direct_motion() fills ref_cache[list][scan8[4]] and 
>> ref_cache[list][scan8[12]] before they are used by pred_motion(). They should 
>> be PART_NOT_AVAILABLE until pred_motion() finishes.
> Nothing uses the mvs in between. Maybe it's faster to avoid interpolating 
> direct mvs in 8x8 blocks that aren't direct mode, but otherwise I won't 
> change it.

I wasn't clear enough. What happens for non intra macroblocks having partition_count == 4?
First subtypes are parsed and if there is a direct subblock, pred_direct_motion() is called.
This call will fill all the ref_cache/mv_cache info for the direct subblocks.
The problem comes afterward. We continue to process the non direct subblocks. First
calling pred_motion() and adding a delta value parsed from the bitstream. Here
pred_motion() calls fetch_diagonal_mv() which depends on the topright ref_cache
if it's PART_NOT_AVAILABLE or not. This is where preceding pred_direct_motion()
could cause trouble (for scan8[4] and scane8[12] positions).

>> 4.
>> Probably a known problem, but b-frame deblocking bs[] calculation based on 
>> mv_cache[] condition is far from standard. I guess it's because b-frames are 
>> mostly non reference frames so minor deblocking bug is not a big problem.
>
> Explain. The only known problems are noted as /*FIXME*/.

You mean the /*FIXME*/ about given frame occupy more reference positions?
Maybe I'am wrong, but I meant another problem.  For b-frames what it does now
(simplified. a,b are the two neighboring positions):

return
  ref[0][a] != ref[0][b]  ||
  abs(mv[0][a].x - mv[0][b].x) >=4 ||
  abs(mv[0][a].y - mv[0][b].y) >=4 ||
  ref[1][a] != ref[1][b] || 
  abs(mv[1][a].x - mv[1][b].x) >=4 ||
  abs(mv[1][a].y - mv[1][b].y) >=4 ||

The correct condition for bs[] =1 is little more complicated. Something like this:

if (ref[0][a] == ref[0][b] && ref[1][a] == ref[1][b]) // same in pairs
{
  if (ref[0][a] == ref[1][a]) // all the same?
  {
    if (abs(mv[0][a].x - mv[1][b].x) <4 &&
       abs(mv[0][a].y - mv[1][b].y) <4 &&
       abs(mv[1][a].x - mv[0][b].x) <4 &&
       abs(mv[1][a].y - mv[0][b].y) <4)
      return 0;
  }
  return 
    abs(mv[0][a].x - mv[0][b].x) >=4 ||
    abs(mv[0][a].y - mv[0][b].y) >=4 ||
    abs(mv[1][a].x - mv[1][b].x) >=4 ||
    abs(mv[1][a].y - mv[1][b].y) >=4;
}
else if (ref[0][a] == ref[1][b] && ref[1][a] == ref[0][b]) // swaped?
{
  return 
    abs(mv[0][a].x - mv[1][b].x) >=4 ||
    abs(mv[0][a].y - mv[1][b].y) >=4 ||
    abs(mv[1][a].x - mv[0][b].x) >=4 ||
    abs(mv[1][a].y - mv[0][b].y) >=4;
}
else
  return 1; // different ref configuration

Again it is rare that this condition returns different result as
the current simplified one. Still this is what the specification sais
and some conformance clips fail.

> Ok, I've never encountered any videos where multiple slices in one frame 
> were different types.

I agree it's very rare. I only saw that in some conformance clips.

>> 5.
> The code is right, the comment was wrong.
> I have not performed a detailed error analysis of the loss in precision, 
> but since it does not appear to introduce any deviation from JM, I have 
> to assume that those lsbs don't actually matter.

My bad, I didn't look into the mmx code carefully. It was suspicious that 
such a factor bug would cause visible artifacts. I didn't actually use the mmx 
code, so it's possible the loss in precision is not a problem.

>> 7.
> fill_default_ref_list() is recalled at the beginning of every slice, 
> whether or not anything has changed.

I thought "default_ref_list_done" prevents calling fill_default_ref_list()
if slice_type doesn't change (in the same frame).

>> 8.
> direct_8x8_inference_flag=1 guarantees that the 4 mvs in a 8x8 block 
> will have the same value, so it doesn't matter which one we use. And they 
> are all always valid, unless that ref list wasn't used, in which case none 
> are valid.

Maybe I was not clear again. Example I didn't say it's about direct_spatial_mv_pred.
I think it possible to have a ref_list[1][0] reference frame with inter MB_TYPE_8x8 
macroblock having 4x4 subblocks using different mv values. Although the actual 
motion vector with direct_spatial_mv_pred is predicted with pred_motion(), 
the ref_list[1][0] motion vectors still play a role on the final vectors.
If they considered non moving they will nullify the predicted motion vector
(when the references fit the conditions). So the problem is which part to 
use from 4 mvs of ref_list[1][0]? At the moment all four are used independently
and hl_motion() uses the topleft mv to do the 8x8 motion compensation.

Sorry about the "not valid" term, I meant not correct values (because
of nullfied motion vectors where it should have predicted or reverse)

bye, Picard