[Mesa-dev] hardware xvmc video decoding with nouveau

[Younes Manton]

2011/8/8 Christian König <deathsimple at vodafone.de>:
> Am Samstag, den 06.08.2011, 14:37 -0400 schrieb Younes Manton:
>> The attached patch I believe should satisfy everyone's needs here. It
>> removes the use of pipe_video_decode_buffer from the state tracker and
>> moves it to the shader decoder. This lets every decoder parse the
>> incoming macroblocks themselves so they can do whatever they need.
> Beside the fact that this patch adds a couple of bugs, you are just
> going back to a design wich has already proven to be a bit flaw full.
>
> Please take a look at these two documents first:
> http://msdn.microsoft.com/en-us/library/ff568441(v=VS.85).aspx
> http://cgit.freedesktop.org/libva/tree/va/va.h
>
> The problem I tried to solve with pipe_video_decode_buffer is that it is
> valid for XvMC to do something like that:
>
> XvMCRenderSurface(target A);
> XvMCRenderSurface(target B);
> XvMCRenderSurface(target A);
> XvMCPutSurface(target A);
> XvMCRenderSurface(target B);
> XvMCPutSurface(target B);
>
> That behaviour creates a bunch of problems when the decoder can only
> work on whole frames, because you can't switch the decoding target while
> inside a frame. This is an ugliness of the XvMC, which in the past made
> it problematic to use it with a couple of hardware decoder.
>
> Most modern players doesn't do it like this any more, but it still seems
> to cause a bunch of problems when seeking or fast forward both with
> mplayer and xine.
>
> So I would suggest we design the interface something like this:

Take a closer look at decode_macroblocks in vl_mpeg12_decoder. Having
the state tracker need to care about how much the decoder is buffering
is a bad idea. I don't see how this is going back to a flawed design,
it's doing almost exactly what is currently done, just not in the
state tracker.

> 1. We have a create_buffer call witch will return just a void pointer.
> This makes it possible for decoders witch needs frame based decoding to
> return an pointer to an internal frame decoding buffer, while it still
> makes it possible for an decoder which is slice based to just return a
> pointer to itself (for example) with this value not being used
> otherwise.

That's not useful, the hardware decoder needs to parse the incoming
macroblocks for itself. It needs to know things like the CBP, motion
type, etc. Your current decode buffer design throws all that info
away. All I did here is pass the macroblocks on to the driver and let
it do the parsing. In the shader case it still dumps stuff to the
decode buffer.

> 2. We have a begin_frame/render/render.../end_frame calls just like
> DXVA, to clearly distinct the start and end of a frame, for frame based
> decoder this makes it possible to do clean housekeeping of the internal
> buffer, while with slice level decoder begin_frame and end_frame are
> just noops.
>
> 3. We pass the render target and the buffer to begin/render/end.
> This enables slice level decoder to start their decoding earlier, while
> it still keeps the burden of figuring out where a frame start/ends to
> the XvMC state tracker.

Again, take a closer look at decode_macroblocks. This is still using
the decode buffer, it's still buffering an entire frame of data, it's
just doing it behind the interface rather than in the state tracker.

>> Too big to inline, see attached. Tested on XvMC/VDPAU with softpipe
>> and an RV620 with a couple of videos (this doesn't imply that any/all
>> combinations produced correct output, just that it was the same
>> before/after).
>
> Beside the design flaws, the patch shows a couple of problems wich needs
> to be addressed first:
>
>> static unsigned MotionVFSToPipe(int xvmc_mo_vfs)
>> {
>> ...
>>    assert(pipe_mo_vfs != 0);
>>
>>    return pipe_mo_vfs;
>>  }
> The motion_vertical_field_select bitfield is part of the mpeg2 spec (see
> section 6.3.17.2). We should stick our definitions to the spec as most
> as possible and don't try to define our own, since XvMC is also using
> this field as defined in the spec it can simple be copied over.
>
> And by the way, it is completely valid for this field to be 0.

Valid for it to be 0 in the case where field-based motion estimation
isn't used as far as I can tell. This never gets called for that case.

>> static enum pipe_mpeg12_macroblock_type TypeToPipe(int xvmc_mb_type)
>> {
>>    if (xvmc_mb_type & XVMC_MB_TYPE_INTRA)
>>       return PIPE_MPEG12_MACROBLOCK_TYPE_INTRA;
>>    if ((xvmc_mb_type & (XVMC_MB_TYPE_MOTION_FORWARD | XVMC_MB_TYPE_MOTION_BACKWARD)) == XVMC_MB_TYPE_MOTION_FORWARD)
>>       return PIPE_MPEG12_MACROBLOCK_TYPE_FORWARD;
>>    /* Workaround for Xine's xxmc video out plugin */
>>    if (!(xvmc_mb_type & ~XVMC_MB_TYPE_PATTERN))
>>       return PIPE_MPEG12_MACROBLOCK_TYPE_FORWARD;
>>    if ((xvmc_mb_type & (XVMC_MB_TYPE_MOTION_FORWARD | XVMC_MB_TYPE_MOTION_BACKWARD)) == XVMC_MB_TYPE_MOTION_BACKWARD)
>>       return PIPE_MPEG12_MACROBLOCK_TYPE_BACKWARD;
>>    if ((xvmc_mb_type & (XVMC_MB_TYPE_MOTION_FORWARD | XVMC_MB_TYPE_MOTION_BACKWARD)) == (XVMC_MB_TYPE_MOTION_FORWARD | XVMC_MB_TYPE_MOTION_BACKWARD))
>>       return PIPE_MPEG12_MACROBLOCK_TYPE_BI;
>>
>>    assert(0);
>>
>>    XVMC_MSG(XVMC_ERR, "[XvMC] Unrecognized mb type 0x%08X.\n", xvmc_mb_type);
>>
>>    return -1;
>> }
> Same problem here, the macroblocktype is defined in section 6.3.17.1 of
> the spec, and like XvMC it just defines forward, backward, pattern and
> intra flags. Since XvMC is also following the spec here this field can
> be copied over without a conversion. For a good reference see the
> VAMacroblockParameterBufferMPEG2 structure in the vaapi specification.
>

True. Good idea.

>> static enum pipe_mpeg12_motion_type MotionToPipe(int xvmc_motion_type, int xvmc_dct_type)
>> {
>>    switch (xvmc_motion_type) {
>>       case XVMC_PREDICTION_FRAME:
>>          if (xvmc_dct_type == XVMC_DCT_TYPE_FIELD)
>>             return PIPE_MPEG12_MOTION_TYPE_16x8;
>>          else if (xvmc_dct_type == XVMC_DCT_TYPE_FRAME)
>>             return PIPE_MPEG12_MOTION_TYPE_FRAME;
>>          break;
>>       case XVMC_PREDICTION_FIELD:
>>          return PIPE_MPEG12_MOTION_TYPE_FIELD;
>>       case XVMC_PREDICTION_DUAL_PRIME:
>>          return PIPE_MPEG12_MOTION_TYPE_DUALPRIME;
>>       default:
>>          assert(0);
>>    }
>>
>>    XVMC_MSG(XVMC_ERR, "[XvMC] Unrecognized motion type 0x%08X (with DCT type 0x%08X).\n", xvmc_motion_type, xvmc_dct_type);
>>
>>    return -1;
>> }
> The distinction between 16x8 and Frame based motion compensation isn't
> the dct type, it is the picture coding, I fixed this bug long ago, and
> you are just going back to the initial broken code here.

Well, if that code was still there for me to refer to I would have
caught it; whatever you fixed before is lost in git history so
apologies if I don't catch it, I'm just going by what I recall things
looking like.

>> struct pipe_mpeg12_picture_desc
>> {
>> ...
>>    uint8_t *intra_quantizer_matrix;
>>    uint8_t *non_intra_quantizer_matrix;
>> ...
>> };
> Both vaapi and DXVA split those out of the picture description into
> separate structures. Honestly I still haven't figured out why, maybe
> this is just to stick more to the spec, but it could be helpful to do
> the same.
>
> So do you have time to work on this a bit more or should I take a break
> from the h264 implementation and try to do it right this time? Since I
> think I now knew what you guys need it should be possible for me to
> change the interface to something everybody is happy with.
>
> Regards,
> Christian.
>
>