[MPlayer-dev-eng] [PATCH] Direct3D libvo driver

[Reimar Döffinger]

On Thu, Nov 13, 2008 at 06:06:32PM +0200, Georgi Petrov wrote:
> 1. if you mistype the double == as a single one like:
> if (FALSE = D3DRecreateContext())
> 
> the compiler will scream at you.

gcc nowadays will at least warn you either way though.
Also as long as quite a few people read the log mails it usually is
seen.

> 2. If you have a long line, you may not see the end or if you scroll
> to the end, and this way it's easier to see if you're comparing a
> failure of the function or success. We don't use 80 character
> limitation, so on long lines, you don't have to scroll that much.
> Before we had to scroll just to see if we're checking GOOD or BAD
> condition.

Now you don't see what you are checking (unless the first part of the
name is enough and that probably means you chose a bad name).
Anyway, nothing worth starting a bikeshed discussion about.

> >> +        /* Copy Y */
> >> +        for (i = 0; i < mpi->height; i++)
> >> +        {
> >> +            fast_memcpy (pScanlineBegin, mpi->planes[0] +
> >> +                         (i * mpi->stride[0]), mpi->stride[0]);
> >> +            pScanlineBegin += stLockedRect.Pitch;
> >> +        }
> >
> > memcpy_pic should do all of that.
> 
> Hmm, yes it does.. I'll update it. Isn't it faster that way instead of
> function call each time? After all this is called each frame...

That maybe 1000 function calls per frame. No, I doubt you can measure
that, it's just a question of code readability (though note that
memcpy_pic has some optimizations when the strides are the same and if
necessary might be improved further in the future).

> >> +        pScanlineBegin = (char *) stLockedRect.pBits;
> >
> > pBits is void * according to my documentation, so that (char *) is
> > not necessary.
> 
> Ok. Does this casting slow down a litle bit?

Uh, no. Casting does not change the resulting binary at all. It's just
about avoiding code that distracts from the real thing going on.
Avoiding them when not necessary also makes it easier to spot the
dangerous casts (e.g. those breaking aliasing or having endianness
issues).

[...]
> Yes, this was left in case I decided to implement error handling
> (reinitialize D3D). May be the handler could sit in Present's failure
> test.

Yes, that was my suggestion, and I am not sure if there is a point in an
additional test when Present fails, you can hardly make anything worse
by reinitializing unconditionally then.

[...]
> Huh... I understand. So - in DRAW_IMAGE I should just copy the frame
> to the OffscreenSurface and the Present should happen in flip_page,
> right? It's like DRAW_IMAGE is called parts of the second before
> flip_page, so the driver has time to copy the frame before flipping?

Yes, I think that basically is the idea. Obviously only helps if DMA is
used somewhere to transfer the data to the graphics card.

> I know about the table - vo_directx.c is like that. I thought it may
> complicate the things by adding another global variable. I hate global
> variables. Unfortunately libvo's logic is based around them...

You are missing the difference between good and bad global variables.
This table would be of the good kind, namely with the type "static
const", which means not visible outside that file (no name collisions
possible) and also it will never be changed at runtime, exactly like code.
I am not aware of any reason to avoid global constant data with
file-level visibility.
But do as you like, I just think without a table it will get ugly soon
after more and more formats are added.

> For example many callback functions are called without parameters,
> counting that the driver has saved them all. It's faster this way, I
> know, but I don't like when I lose proximity.

_That_ is due to bad design, not speed considerations.

> > Would be better not to set VFCAP_OSD as long as you do not really
> > support it, that will allow vf_expand to draw the OSD.
> 
> Hah!!! This is great. I will! I will implement subtitles later, of
> course. Does vf_expand handle this by default (does it get into the
> filter chain), or I should add it like -vf expand?

Hm, I assumed it was supposed to be added automatically.
But going by how "-vo gl:noosd" behaves you will have to use "-vf
expand=osd=1"

> > The breaks are pointless if you have a return. Since this is already so
> > much boilerplate code I'd appreciate it if you could remove them.
> 
> Hmmm, it depends. What is I decide to do something else afterward and
> forget to return something... I always think that the right "case
> framework" is:
> 
> case xxxxxx:
>     some code...
> break;

I know this is not a clear-cut case, my reason for disliking it
especially in this case is that here it means almost 30% of code is
"useless" which puts a dent on readability.
I'm not telling you what to do here, just saying I dislike it.

> >> +/** @brief libvo Callback: Unused function
> >> + *  @return N/A
> >> + */
> >> +static int draw_slice(uint8_t *src[], int stride[], int w,int h,int x,int y )
> >> +{
> >> +    mp_msg(MSGT_VO,MSGL_ERR,"<vo_d3d>draw_slice called\n");
> >> +    return VO_ERROR;
> >> +}
> >
> > This probably is wrong, I think draw_slice is almost always called for any YUV
> > data, and you must copy the data there since DRAW_FRAME only gets and
> > mpi with (mpi->flags & MP_IMGFLAG_DRAW_CALLBACK) and no actual image
> > data.
> 
> I don't understand that. With both YV12/YUY2 colorspaces this function
> never gets called. What's the difference between it and DRAW_FRAME???

The difference is it (possibly) updates only a part of the image (a
"slice", not all of it. This means there is a good chance that the whole
thing fits in the cache and all processing on it can be done without
reading from main memory.
And this function will get called when you play an MPEG-2 file e.g.
http://samples.mplayerhq.hu/MPEG2/dothack2.mpg (without
any video filters!) and you will only get a black image if you do
not implement that function (unless you use -noslices).

> And when (mpi->flags & MP_IMGFLAG_DRAW_CALLBACK) is true and when
> false?

It is true when draw_slice was already called, so the data has
(hopefully) already been copied to the graphics card and so there
usually is no more work to do during VOCTRL_DRAW_IMAGE (which is what I
meant all along when I said DRAW_FRAME, the draw_frame function is
indeed not used when you implemented VOCTRL_DRAW_IMAGE).

> But first - should I fix them first or the patch
> will get accepted?

Fix them first, I did not review at least your window management code
since I hope w32_common can replace it, and unreviewed code IMO should
not be committed.

> And what will happen after the patch is accepted?
> Am I going to post further modifications to vo_direct3d.c here ot I'll
> have read-write access to commit whenever I want? I just want to know
> in order to prepare "commit technique".

Prepare to post them here. Even if you get read-write access right away
I think you haven't sent any patches yet and it probably doesn't hurt to
see what we complain about so you don't get flamed for your first commit
;-) (don't worry about flames, they don't hurt they just might annoy
greatly).

Greetings,
Reimar Döffinger