[MPlayer-dev-eng] [PATCH] Support for XvMC VLD on S3 Unichrome

[Ivan Kalvachev]

On Feb 16, 2008 3:43 AM, Timothy Lee <timothy.lee at siriushk.com> wrote:
> Compn wrote:
> > On Sat, 5 Jan 2008 10:21:33 +0000 (UTC), Carl Eugen Hoyos wrote:
> >
> >> Timothy Lee <timothy.lee <at> siriushk.com> writes:
> >>
> >>> It would be great to get XvMC VLD support on the trunk.   Looking
> >>> forward to your comments!
> >>>
> >> Ivan, did you see this mail? Could you comment?
> >>
> >> Carl Eugen
> >>
> > iive said he is writing a big review for it, please wait
> >
> > -compn
> >
> How's the review coming along?

Sorry for the delay I wanted to check few things, but time never was enough.

Ok, patch is rejected. Same reason as the original one.
FFmpeg should not call XvMC functions directly.

The biggest issue seems to be qmatrix and binfo (XvMCQMatrix &
XvMCMpegControl).  I think that these belong to the xvmc_render
structure (per surface).  They should be set by the codec and the vo
would call the XvMC functions they are destined to, on its first run
of draw_slice() .
I'm not sure if xvmc_render should include them fully, pointer to
shared or pointer to malloc-ed, it's up to you to deside:)

Now about the changes and improvements that could be done:
(file by file, it may be a little chaotic, I didn't want to post the
full patch).

* xvmcvideo.c:
- I cannot understand what findPastSurface() and findFutureSurface()
do. They just seem to dublicate what ffmpeg12 already does. The frames
and surfaces types are well know by the codec. Also they do some
checks that are kind of strange . E.g. they check if the prediction
frame is B-picture. The only way this could happen is in interlace
video when the second decoded field is B, but in this case the other
field is still valid source for prediction (afair).

- In render->flags you put XVMC_TOP/BOTTOM_FIELD / XVMC_FRAME_PICTURE.
It is kind of strange as I don't see where these flags are used at all
and the render->picture_structure already have that info. The
picture_structure is argument to non-vld XvMCRenderSurface(). BTW you
can just render->flags|=render->picture_structure;


- XvMC_field_end() - no calling of XvMC functions  inside ffmpeg
should be done. In this one XvMC already requires that
avctx->draw_horiz_band() to be called and the flush part is already
performed there.
At least in the standard part, the sync is blocking function that
incorporates flush in it. Blocking while video card works kills all
possibility for parallel work of card and CPU.

- length_to_next_start() - I think libavcodecs already have optimized
function for startcode lookup, use it.

- xvmc_decode_slice() - hum, isn't there way to reuse the existing fields. Like
mv_blocks instead of slice_data, start_mv_block_num for startcode. I'm
not against adding new fields, just wondering.

* mpeg12.c
- extern doesn't need #ifdef

- You can just add MPEG2_VLD to pixfmt_xvmc_mpg2_420[], no need for
new structure.

Also you don't need special mpeg_xxmc_decode_init() and another
decoder for it, the pixel format would be queried by the get_format()
and above array and vld mode be used if get_format() accepts it.

* vd_ffmpeg.c
- why all these hacks with voname? This hack won't be accepted. There
is easier way to fall back to software decoder, using the get_format()
call inside ffmpeg. It's been in my todo list for quite some time. (if
you do it, send as separate patch).

- always use if(){}else{} even when there is only one statement in the {} body.

- stheader.h - huh? where this came from?

* vo_xvmc.c
- check for  "img_format== ..." instead of using hasVLDAcceleration()
or at least make it macro.

- the typo in the text string could be committed right away, I just
need to know to whom to give credit for it :)

-check_xvmc_vld() looks kind of familiar, but I cannot say from where ;)

For now I think this is enough.
Happy hacking.