[PATCH 3/5] winegstreamer: Implement WMA decoder ProcessInput.

[Zebediah Figura (she/her)]

On 2/23/22 02:32, Rémi Bernon wrote:
>> Why are we holding a reference to the media buffer? (And if we need to,
>> should we be storing that instead of the IMFSample?)
>>
> 
> 
> I think we have to keep a reference on the buffer, because Lock doesn't,
> and because we keep the buffer data pointer around.
> 
> It's probably unnecessary, and if someone modifies the sample while
> we're holding it, removing or adding buffers, it'll all probably crash
> but it felt more correct to keep a buffer reference.

I guess so. It does look like the sample object is supposed to keep a
reference to its own buffers, but I guess someone could manhandle the
buffers while we're using them. On the other hand, if we care about
that, we're currently keeping a reference to one buffer but releasing a
reference to what could be another, which doesn't look right by any
definition.

> We'll need the pointer to the IMFSample later to be able to update its
> properties, such as timestamp and durations.

Okay, makes sense.

> As I used a single private pointer I had to chose which one to store,
> but I later added a wrapper struct so maybe I can put the pointers there
> instead.
> 
> 
>>> +
>>> +        IMFMediaBuffer_Release(media_buffer);
>>> +    }
>>> +
>>> +    IMFSample_Release(sample->private);
>>> +    free(sample);
>>> +}
>>> diff --git a/dlls/winegstreamer/unixlib.h b/dlls/winegstreamer/unixlib.h
>>> index 4adbb694766..59b15df7d08 100644
>>> --- a/dlls/winegstreamer/unixlib.h
>>> +++ b/dlls/winegstreamer/unixlib.h
>>> @@ -103,6 +103,14 @@ struct wg_format
>>>       } u;
>>>   };
>>>   +struct wg_sample
>>> +{
>>> +    UINT32 max_size;
>>> +    UINT32 size;
>>> +    BYTE *data;
>>> +    void *private;
>>> +};
>>
>> "max_size" isn't relevant until patch 5/5.
>>
> 
> 
> Sure.
> 
> 
>> "private" also doesn't particularly seem like it belongs here. In
>> particular, if you're going to wrap the wg_sample in another structure
>> in patch 5/5, I'd rather see the same approach taken for
>> frontend-specific data.
>>
> 
> 
> I used it on the unix side when re-using the struct for internal sample
> passing, but I guess I can also wrap it there, it'll probably make
> things cleaner.
> 
> 
>>> +    if (FAILED(hr = IMFSample_ConvertToContiguousBuffer(sample,
>>> &media_buffer)))
>>> +        return hr;
>>> +    IMFMediaBuffer_Release(media_buffer);
>>
>> This seems pointless (and hence, at least, doesn't belong in this
>> patch). Or does ConvertToContiguousBuffer() also modify the original
>> sample?
>>
> 
> 
> It does, and it makes sure there's a single buffer in the sample, as
> wg_sample_create only support that case.

Ah, okay, I guess on reflection it would have to.

FWIW, it seems it's legal to call that function with a NULL argument.

> 
> 
>>> +    if (!(wg_sample->size = (wg_sample->size / info.cbSize) *
>>> info.cbSize))
>>> +    {
>>> +        mf_destroy_wg_sample(wg_sample);
>>> +        return S_OK;
>>> +    }
>>
>> I can't tell if this is trying to align to info.cbSize or (as the
>> documentation specifies) treat it as a minimum, but either way I think
>> it's broken.
>>
> 
> 
> Testing shows that pushing buffers that aren't multiple of the input
> sample size truncates the data to the nearest sample size, and if the
> sample size ends up being 0, just returns S_OK and do nothing.
> 
> This is checked by some of the mf tests.

Ah, okay. I was about to ask you to add a comment for that, but I see
you've already changed the code in v2 :-)

> 
> 
>>> +
>>> +    decoder->input_sample = wg_sample;
>>
>> Do we have to store this for API compatibility? If so it would seem
>> helpful to specify that in a comment. It looks wrong as-is, since we
>> don't need the data after we copy it.
>>
> 
> 
> We do, for the tests to pass at least.

Right, but it's not clear to me how important it is that we have those
tests, or that they pass.

> 
> 
>> (And depending on how hard we need to preserve API compatibility in this
>> area, I'm concerned that patch 5/5 may be making assumptions about how
>> GStreamer uses buffers that aren't actually guaranteed.)
> 
> 
> I'm not sure what your concerns are exactly?
> 

Well, GStreamer isn't actually guaranteeing that we'll get one sample
output synchronously per sample input, or that the input sample will be
released synchronously, or even that it'll be reused in-place. If
applications are depending on any of these, that'll be hard to follow,
and harder if we let GStreamer dictate when the input IMFSample is released.

I'm guessing it's not really that important anyway (and if GStreamer
does behave differently, there's not much we can do in general...)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 495 bytes
Desc: OpenPGP digital signature
URL: <http://www.winehq.org/pipermail/wine-devel/attachments/20220223/794a8068/attachment.sig>