[MPlayer-dev-eng] [PATCH] - No Reset of D3D device on resize

[Reimar Döffinger]

Ok, just for reference you will find the whole long mail I started to
write, feel free to read it or skip is as you like, but
honestly there is just one major thing that I think is wrong with the
patch:
Why on earth did you remove the destroy_d3d_surfaces() and replaced all
the places where it was called with at least 10 lines of code that do
_exactly_ the same thing!?!!
(ignoring the configure_d3d that ends up releasing the device first and
the associated backbuffer etc. only after that, and the same question
applies to create_d3d_surfaces(), and yes I know both functions are
named badly).


On Wed, Dec 24, 2008 at 06:50:44PM +0200, Georgi Petrov wrote:
> >> Hey, did you forget the patch. Just checking :)
> >
> > No, but I had not much time and there were places that looked very
> > suspicious.
> 
> Ok, I understand that you don't have much time. I develop for MPlayer
> in my free time as well, but if you find anything suspicious, please
> let me know when you discover it. I suppose you didn't check the
> patch, decided it was suspicious and left it behind without notifying
> me, but you just didn't have time to check it at all :)

Well, what was and is suspicious to me is that the patch is about 3-4
times as large as I expected it.

> > You are duplicating the handling of d3d_surface and d3d_backbuf
> > deallocation in three places which is already quite ugly.
> 
> No, they shouldn't go hand in hand anymore. Now we have 3 "types" of
> surfaces/textures:
> 
> 1. The offscreen surface.
> 2. The backbuffer surface.
> 3. The OSD textures.
> 
> The first one is never recreated except when we do a device reset,
> because it's always equal to the movie's dimensions. To do a Reset,
> all allocated resources should be freed. Otherwise we would keep it as
> well.
> 
> The backbuffer is never recreated except when growing the window
> larger than fullscreen (the backbuffer is now set to the fullscreen
> initially).

Uh, Reset is called exactly when the backbuffer is grown, so these two
cases are the same...

> The OSD textures should always match current window dimensions, so
> they are freed/recreated on each resize.

And only that case is a different one, so I would have thought you could
just have created extra functions to destroy/create the OSD texture and
left everything else the same...
Or to put it more clearly: The main purpose of the patch is to change
the behaviour during resizing, but the vast majority of changes is in
code that is in no way at all related to resizing.
For example, with this patch configure_d3d suddenly also frees some
stuff.
One issue with that is that the doxygen no longer matches the code, but
more important to me is: Do you actually yourself still know what the
_purpose_ of the function is (describe-able in a few words) and how
its functionality differs from other functions?
If you just thought up such a description, does it explain why
calc_fs_rect() is called?
Btw. while looking through the code under this aspect I realized
that the IDirect3D9_GetDeviceCaps in preinit really makes no sense,
since the D3DADAPTER_DEFAULT in preinit is not necessarily the one we
will be rendering on in the end.

> > IMO it should be a simple
> > if (vo_dwidth > priv->cur_backbuf_width || vo_dheight ...) {
> >  update cur_backbuf_*
> >  same code as the current one
> > } else {
> >  destroy_d3d_osd_textures();
> >  create_d3d_osd_textures();
> > }
>
[...]
> You see, at the beginning of the function we have exactly what do you
> propose:
> 
>     if (vo_dwidth <= priv->cur_backbuf_width &&
>         vo_dheight <= priv->cur_backbuf_height)
>         return 1;
> 
> So i exits if the new dimensions are smaller than the backbuffer, or
> does a full Reset if they are larger.

You misunderstood my complaint, besides the OSD textures not being
freed, my complaint was that you wrote a _new_ _function_ called
check_d3d_backbuffer_dimensions although the _current_ resize_d3d
already did everything necessary (in a way that was already tested and
reviewed) for this case and you would only have had to _add_ some code
to handle the _new_ _case_ (resizing without reset).
Or to express it differently: I do somewhat fail to see why to add a new
feature you rewrote code that handles existing features that do not
change.
Unless you thought the code was so badly designed that it is impossible to add new
features without rewriting the existing code (which I don't think is the
case, though improvements are always possible in that area).
I will admit that the check_d3d_backbuffer_dimensions would have used
the current code almost unchanged if you hadn't removed the
destroy_d3d_surfaces function.

Greetings,
Reimar Döffinger