[FFmpeg-devel,2/3] kmsgrab: Do not require the modifier to stay constant.

As we get a new set of objects each frame anyway, we
do not gain anything by keeping the modifier constant.

This helps with capturing when switching your setup a
bit, e.g. from ingame to desktop or from X11 to wayland.
---
 libavdevice/kmsgrab.c | 7 -------
 1 file changed, 7 deletions(-)

On 03/11/2020 23:17, Bas Nieuwenhuizen wrote:
> As we get a new set of objects each frame anyway, we
> do not gain anything by keeping the modifier constant.
> 
> This helps with capturing when switching your setup a
> bit, e.g. from ingame to desktop or from X11 to wayland.
> ---
>   libavdevice/kmsgrab.c | 7 -------
>   1 file changed, 7 deletions(-)
> 
> diff --git a/libavdevice/kmsgrab.c b/libavdevice/kmsgrab.c
> index 2a03ba4122..8b698b7f30 100644
> --- a/libavdevice/kmsgrab.c
> +++ b/libavdevice/kmsgrab.c
> @@ -176,13 +176,6 @@ static int kmsgrab_get_fb2(AVFormatContext *avctx,
>           err = AVERROR(EIO);
>           goto fail;
>       }
> -    if (fb->modifier != ctx->drm_format_modifier) {
> -        av_log(avctx, AV_LOG_ERROR, "Plane %"PRIu32" framebuffer "
> -               "format modifier changed: now %"PRIx64".\n",
> -               ctx->plane_id, fb->modifier);
> -        err = AVERROR(EIO);
> -        goto fail;
> -    }
>       if (fb->width != ctx->width || fb->height != ctx->height) {
>           av_log(avctx, AV_LOG_ERROR, "Plane %"PRIu32" framebuffer "
>                  "dimensions changed: now %"PRIu32"x%"PRIu32".\n",
> 

I included this case because it's really a format change and may not be compatible with something happening later.

For example: on Intel you might be capturing a Y-tiled framebuffer and feeding it directly (no copy step) to an encoder via VAAPI.  If it then switches to linear most things will continue to work (import is still fine), but the encoder will barf in an opaque way because it doesn't support non-Y-tiled inputs.

Is that actually helpful?  I'm not sure.

Can you explain a bit more about what is changing in the switching cases you are thinking of, so that the tradeoffs are maybe a bit clearer?

Thanks,

- Mark

On Wed, Nov 4, 2020 at 1:09 AM Mark Thompson <sw@jkqxz.net> wrote:
>
> On 03/11/2020 23:17, Bas Nieuwenhuizen wrote:
> > As we get a new set of objects each frame anyway, we
> > do not gain anything by keeping the modifier constant.
> >
> > This helps with capturing when switching your setup a
> > bit, e.g. from ingame to desktop or from X11 to wayland.
> > ---
> >   libavdevice/kmsgrab.c | 7 -------
> >   1 file changed, 7 deletions(-)
> >
> > diff --git a/libavdevice/kmsgrab.c b/libavdevice/kmsgrab.c
> > index 2a03ba4122..8b698b7f30 100644
> > --- a/libavdevice/kmsgrab.c
> > +++ b/libavdevice/kmsgrab.c
> > @@ -176,13 +176,6 @@ static int kmsgrab_get_fb2(AVFormatContext *avctx,
> >           err = AVERROR(EIO);
> >           goto fail;
> >       }
> > -    if (fb->modifier != ctx->drm_format_modifier) {
> > -        av_log(avctx, AV_LOG_ERROR, "Plane %"PRIu32" framebuffer "
> > -               "format modifier changed: now %"PRIx64".\n",
> > -               ctx->plane_id, fb->modifier);
> > -        err = AVERROR(EIO);
> > -        goto fail;
> > -    }
> >       if (fb->width != ctx->width || fb->height != ctx->height) {
> >           av_log(avctx, AV_LOG_ERROR, "Plane %"PRIu32" framebuffer "
> >                  "dimensions changed: now %"PRIu32"x%"PRIu32".\n",
> >
>
> I included this case because it's really a format change and may not be compatible with something happening later.
>
> For example: on Intel you might be capturing a Y-tiled framebuffer and feeding it directly (no copy step) to an encoder via VAAPI.  If it then switches to linear most things will continue to work (import is still fine), but the encoder will barf in an opaque way because it doesn't support non-Y-tiled inputs.

It sounds like the actual modifier is the problem though and not the
change of modifiers? Is there better error handling for the initial
modifier vs. a new modifier?
>
> Is that actually helpful?  I'm not sure.
>
> Can you explain a bit more about what is changing in the switching cases you are thinking of, so that the tradeoffs are maybe a bit clearer?

So Intel/AMD on modern-ish GPUs support compressing the images on
demand via some kind of custom compression (DCC on AMD, CCS on Intel).
These need an extra memory plane for their compression status. However
this shouldn't be used for frontbuffer rendering because the main &
metadata planes are guaranteed to get out of sync during rendering
(lack of cache coherency).

So what you see:

1) X11 normally uses a non-compressed modifier
2) Most wayland compositors use a compressed modifier.
3) X11 when it is doing pageflipping from an application directly
(i.e. fullscreen but it may happen with some compositors as well) may
use a compressed modifier.

In particular switching between 1 and 3 seems reasonable for an user
to do (1 when setting up their recording and then 3 while recording a
game).

>
> Thanks,
>
> - Mark

On 04/11/2020 00:26, Bas Nieuwenhuizen wrote:
> On Wed, Nov 4, 2020 at 1:09 AM Mark Thompson <sw@jkqxz.net> wrote:
>>
>> On 03/11/2020 23:17, Bas Nieuwenhuizen wrote:
>>> As we get a new set of objects each frame anyway, we
>>> do not gain anything by keeping the modifier constant.
>>>
>>> This helps with capturing when switching your setup a
>>> bit, e.g. from ingame to desktop or from X11 to wayland.
>>> ---
>>>    libavdevice/kmsgrab.c | 7 -------
>>>    1 file changed, 7 deletions(-)
>>>
>>> diff --git a/libavdevice/kmsgrab.c b/libavdevice/kmsgrab.c
>>> index 2a03ba4122..8b698b7f30 100644
>>> --- a/libavdevice/kmsgrab.c
>>> +++ b/libavdevice/kmsgrab.c
>>> @@ -176,13 +176,6 @@ static int kmsgrab_get_fb2(AVFormatContext *avctx,
>>>            err = AVERROR(EIO);
>>>            goto fail;
>>>        }
>>> -    if (fb->modifier != ctx->drm_format_modifier) {
>>> -        av_log(avctx, AV_LOG_ERROR, "Plane %"PRIu32" framebuffer "
>>> -               "format modifier changed: now %"PRIx64".\n",
>>> -               ctx->plane_id, fb->modifier);
>>> -        err = AVERROR(EIO);
>>> -        goto fail;
>>> -    }
>>>        if (fb->width != ctx->width || fb->height != ctx->height) {
>>>            av_log(avctx, AV_LOG_ERROR, "Plane %"PRIu32" framebuffer "
>>>                   "dimensions changed: now %"PRIu32"x%"PRIu32".\n",
>>>
>>
>> I included this case because it's really a format change and may not be compatible with something happening later.
>>
>> For example: on Intel you might be capturing a Y-tiled framebuffer and feeding it directly (no copy step) to an encoder via VAAPI.  If it then switches to linear most things will continue to work (import is still fine), but the encoder will barf in an opaque way because it doesn't support non-Y-tiled inputs.
> 
> It sounds like the actual modifier is the problem though and not the
> change of modifiers? Is there better error handling for the initial
> modifier vs. a new modifier?

Indeed.  The only difference is that working out why it failed is probably a bit easier in the first case.

>>
>> Is that actually helpful?  I'm not sure.
>>
>> Can you explain a bit more about what is changing in the switching cases you are thinking of, so that the tradeoffs are maybe a bit clearer?
> 
> So Intel/AMD on modern-ish GPUs support compressing the images on
> demand via some kind of custom compression (DCC on AMD, CCS on Intel).
> These need an extra memory plane for their compression status. However
> this shouldn't be used for frontbuffer rendering because the main &
> metadata planes are guaranteed to get out of sync during rendering
> (lack of cache coherency).
> 
> So what you see:
> 
> 1) X11 normally uses a non-compressed modifier
> 2) Most wayland compositors use a compressed modifier.
> 3) X11 when it is doing pageflipping from an application directly
> (i.e. fullscreen but it may happen with some compositors as well) may
> use a compressed modifier.
> 
> In particular switching between 1 and 3 seems reasonable for an user
> to do (1 when setting up their recording and then 3 while recording a
> game).

There isn't anything making them use the same framebuffer format here, is there?  (It doesn't need a new modeset.)  I guess they will typically use the same logic and so if everyone stays in 8-bit BGRX it doesn't matter.

I think I'm convinced in any case, so LGTM.

Thanks,

- Mark