[vlc-devel] [PATCH 1/3] vdpau/display: remove unused has_pictures_invalid

[Steve Lhomme]

On 17/01/2019 16:59, Rémi Denis-Courmont wrote:
> Le torstaina 17. tammikuuta 2019, 16.34.08 EET Steve Lhomme a écrit :
>> On 16/01/2019 16:59, Rémi Denis-Courmont wrote:
>>> Le keskiviikkona 16. tammikuuta 2019, 14.33.52 EET Steve Lhomme a écrit :
>>>> OK so the documentation of "has_pictures_invalid" needs to be changed.
>>>> vout_display_SendEventPicturesInvalid() indirectly calls
>>>> VOUT_DISPLAY_RESET_PICTURES which is also what happens when a
>>>> resize/crop/etc error occurs but synchronously.
>>>>
>>>> So has_pictures_invalid means the display handles
>>>> VOUT_DISPLAY_RESET_PICTURES.
>>> It rather means that the display may need VOUT_DISPLAY_RESET_PICTURES.
>>> Though in practice, it only really affects pool allocation, and should be
>>> irrelevant if the display uses push-buffers.
>> "if the display uses push buffers" yes. But there are some cases where
>> it's not possible. In many display modules the display pool has its own
>> lock/unlock callbacks.
> I stopped reading here.

That's not a correct way of handling comments on your code. I was 
explaining the changes you made on VOUT_DISPLAY_CHANGE_xxx to reset the 
display introduced the error you complained about. My code just exhibits 
the issue more often.

Also in general ignoring an error is a bad coding practice. I am 
proposing to handle the case where resizing a buffer fails (which can 
happen, since it happened to you).

> Lock/unlock has been flaky, not to say plainly broken,
> for as long as I remember - which is at least as long as picture_pool_t
> existed. I don't recall if/how the callbacks worked before.
The lock/unlock of the memory is still going to be needed. It may be 
possible to fake direct rendering in these (if you read the whole thing 
you would have seen it's only an issue in "indirect" rendering) and do 
the lock/copy/unlock during the Prepare. Many of these modules 
(direct3d9, direct3d11, android opengl/display, mmal) currently do it on 
more than one picture though. There could be tearing issues if there's 
no double buffering involved. It will have to be handled internally by 
each module.

So in the long run it may be better, I don't think it's mandatory change 
needed for the push model. At least it works without for me.

BTW, I mentioned that mmal handles lock and VOUT_DISPLAY_RESET_PICTURES 
but that's not the case.