[Mesa-dev] [PATCH 2/3] xorg/xvmc: Only set decode buffer when available

[Younes Manton]

On Sun, Aug 28, 2011 at 12:56 PM, Maarten Lankhorst
<m.b.lankhorst at gmail.com> wrote:
> On 08/28/2011 06:23 PM, Younes Manton wrote:
>> On Sun, Aug 28, 2011 at 12:13 PM, Younes Manton <younes.m at gmail.com> wrote:
>>> On Sat, Aug 27, 2011 at 7:58 PM, Maarten Lankhorst
>>> <m.b.lankhorst at gmail.com> wrote:
>>>> The nouveau xvmc decoder doesn't need it.
>>>>
>>>> Signed-off-by: Maarten Lankhorst <m.b.lankhorst at gmail.com>
>>>> ---
>>>>  src/gallium/state_trackers/xorg/xvmc/surface.c |    9 ++++++---
>>>>  1 files changed, 6 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/src/gallium/state_trackers/xorg/xvmc/surface.c b/src/gallium/state_trackers/xorg/xvmc/surface.c
>>>> index 79bd9c6..9cfec94 100644
>>>> --- a/src/gallium/state_trackers/xorg/xvmc/surface.c
>>>> +++ b/src/gallium/state_trackers/xorg/xvmc/surface.c
>>>> @@ -111,7 +111,8 @@ SetDecoderStatus(XvMCSurfacePrivate *surface)
>>>>    context_priv = surface->context->privData;
>>>>    decoder = context_priv->decoder;
>>>>
>>>> -   decoder->set_decode_buffer(decoder, surface->decode_buffer);
>>>> +   if (surface->decode_buffer)
>>>> +      decoder->set_decode_buffer(decoder, surface->decode_buffer);
>>>>    decoder->set_decode_target(decoder, surface->video_buffer);
>>>>
>>>>    for (i = 0; i < 2; ++i) {
>>>> @@ -181,7 +182,8 @@ Status XvMCCreateSurface(Display *dpy, XvMCContext *context, XvMCSurface *surfac
>>>>    if (!surface_priv)
>>>>       return BadAlloc;
>>>>
>>>> -   surface_priv->decode_buffer = context_priv->decoder->create_buffer(context_priv->decoder);
>>>> +   if (context_priv->decoder->create_buffer)
>>>> +      surface_priv->decode_buffer = context_priv->decoder->create_buffer(context_priv->decoder);
>>>>    surface_priv->video_buffer = pipe->create_video_buffer
>>>>    (
>>>>       pipe, PIPE_FORMAT_NV12, context_priv->decoder->chroma_format,
>>>> @@ -496,7 +498,8 @@ Status XvMCDestroySurface(Display *dpy, XvMCSurface *surface)
>>>>       SetDecoderStatus(surface_priv);
>>>>       context_priv->decoder->end_frame(context_priv->decoder);
>>>>    }
>>>> -   context_priv->decoder->destroy_buffer(context_priv->decoder, surface_priv->decode_buffer);
>>>> +   if (surface_priv->decode_buffer)
>>>> +      context_priv->decoder->destroy_buffer(context_priv->decoder, surface_priv->decode_buffer);
>>>>    surface_priv->video_buffer->destroy(surface_priv->video_buffer);
>>>>    FREE(surface_priv);
>>>>    surface->privData = NULL;
>>>> --
>>>> 1.7.6
>>>>
>>>>
>>>> _______________________________________________
>>>> mesa-dev mailing list
>>>> mesa-dev at lists.freedesktop.org
>>>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>>>>
>>> Pushed. Thanks.
>>>
>> Oops, sorry, the above was referring to the xvmc test one. For this
>> one maybe it's better if you implement create_buffer by just returning
>> something non-null (like your decoder object), and ignoring it in
>> set/destroy that way we don't have to have to repeat these checks in
>> every state tracker. I don't feel strongly either way though. What do
>> you think?
> I felt it to be cleaner to just skip in that case, I think it might make more
> sense to just remove those calls entirely, since the decoder should
> handle it internally. Since all those set_decode_buffer and set_decode_targets
> are always paired, is there any reason why the decoder shouldn't?
>
> Cheers,
> Maarten
>

Well, that was what the last discussion was all about, whether or not
decode buffers should be handled internally by each driver or
externally. It was decided to handle it externally, to simplify life
for drivers that want/need multiple buffers and to keep most of the
ugliness in XvMC, since VDPAU and VA don't have the same kinds of
problems.

Anyway, your patch is cleaner for the driver that doesn't want to
support multiple decode buffers, but now every state tracker has to
deal with drivers with decode buffers and drivers without, and we have
to do these if checks at init/cleanup and every frame. The alternative
is that you support create_buffer, etc, and just return the same
decode buffer each time, and if you don't want to bother creating a
new type to represent a decode buffer you can just return anything
non-null, like the decoder itself, and just implement the other
functions as emty no-ops, that way the state trackers only have to
deal with one interface.

Anyway, I don't have a strong preference either way, and since we
currently only have 2 drivers and 2 state trackers, it doesn't matter
much. I'll push this in a couple of days if no other comments.