[MPlayer-dev-eng] [PATCH] Direct3D VO better handling of uncooperative video adapter

[Georgi Petrov]

On Tue, Feb 3, 2009 at 12:51 PM, Reimar Döffinger
<Reimar.Doeffinger at stud.uni-karlsruhe.de> wrote:
> On Tue, Feb 03, 2009 at 12:43:03PM +0200, Georgi Petrov wrote:
>> On Tue, Feb 3, 2009 at 12:24 PM, Reimar Döffinger
>> <Reimar.Doeffinger at stud.uni-karlsruhe.de> wrote:
>> > Also have you thought about but ensure_..._created/ensure_..._freed
>> > system? Currently it is possible (though unlikely) that we have e.g. a
>> > valid d3d_device but not video texture.
>>
>> Hmm, most likely this is true. I'll have to review the whole code very
>> carefully or even change a little bit the code to remove those
>> half-initialized possible states.
>
> Well, my idea with those ensure_ function was to do exactly that, but in
> a way that does not lead to the same kind of IMO chaotic and hard to
> understand code we currently have (mostly this would be due to
> documenting the possible initialization states and have
> consistently-named functions to transition between them).
> Also by using ensure_..._created a context would be recreated as soon as
> possible/needed, not only in flip_page.
>
>> The patch as it is now is nevertheless needed, because it only adds
>> checks, which are good to have anyways.
>
> Yes, I don't object to it, my suggestion was just a more complete way,
> and one that is easier to verify for correctness.
>

Ok, I agree and I'll work on this issue these days.

>> >> @@ -857,21 +862,16 @@
>> >>  static void flip_page(void)
>> >>  {
>> >>      RECT rect = {0, 0, vo_dwidth, vo_dheight};
>> >> -    if (FAILED(IDirect3DDevice9_Present(priv->d3d_device, &rect, 0, 0, 0))) {
>> >> +    if (!priv->d3d_device ||
>> >> +        FAILED(IDirect3DDevice9_Present(priv->d3d_device, &rect, 0, 0, 0))) {
>> >>          mp_msg(MSGT_VO, MSGL_V,
>> >> -               "<vo_direct3d>Video adapter became uncooperative.\n");
>> >> -        mp_msg(MSGT_VO, MSGL_ERR, "<vo_direct3d>Trying to reinitialize it...\n");
>> >> +               "<vo_direct3d>Trying to reinitialize uncooperative video adapter.\n");
>> >>          if (!reconfigure_d3d()) {
>> >> -            mp_msg(MSGT_VO, MSGL_V, "<vo_direct3d>Reinitialization Failed.\n");
>> >> +            mp_msg(MSGT_VO, MSGL_V, "<vo_direct3d>Reinitialization failed.\n");
>> >>              return;
>> >>          }
>> >> -        if (FAILED(IDirect3DDevice9_Present(priv->d3d_device, &rect, 0, 0, 0))) {
>> >> -            mp_msg(MSGT_VO, MSGL_V, "<vo_direct3d>Reinitialization Failed.\n");
>> >> -            return;
>> >> -        }
>> >
>> > Why are you removing it? I guess it can't work, but if so it should be
>> > done in a separate patch with the reason explained in the commit
>> > message.
>>
>> In flip_page() it was the same - a Present was issued on a possibly
>> invalid d3d_device and THIS was the ultimate reason MPlayer used to.
>
> Yes, but if that was the only issue, the right solution would be to add
> priv->d3d_device && ...
>

No. The current code (before the patch) is this:

if (FAILED(IDirect3DDevice9_Present(priv->d3d_device, &rect, 0, 0, 0)))

It's flaw is not checking if priv->d3d_device is NULL.

After the patch it is:

if (!priv->d3d_device ||
    FAILED(IDirect3DDevice9_Present(priv->d3d_device, &rect, 0, 0, 0)))

It first checks if priv->d3d_device is NULL and if it is NULL, no
Present on a null pointer is made, but instead the reinitialization is
done right away. If priv->d3d_device is not NULL, then a Present is
made and if it fails for some reason, reinitialization is performed.

If we change it to &&, then in case priv->d3d_device is NULL, the
second if condition will be probed, which will result in Present on a
NULL pointer.