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

[Rémi Bernon]

On 2/23/22 03:09, Zebediah Figura (she/her) wrote:
> Much of this patch doesn't make sense without 4/5. Could the two patches
> be squashed together?
> 
> 
> 
> On 2/22/22 16:48, Rémi Bernon wrote:
>> +    if (FAILED(hr = IMFSample_GetBufferCount(sample, &count)) || count != 1)
>> +    {
>> +        FIXME("Only samples with 1 buffer are supported!\n");
>> +        return hr ? hr : E_FAIL;
>> +    }
> 
> This made me do a double take. Since these conditions don't really share
> their bodies, perhaps they should be separated?
> 


Sure, I wanted to keep it short as it's not supposed to happen anyway.


>> +void mf_destroy_wg_sample(struct wg_sample *sample)
>> +{
>> +    IMFMediaBuffer *media_buffer;
>> +    HRESULT hr;
>> +
>> +    if (FAILED(hr = IMFSample_GetBufferByIndex(sample->private, 0, &media_buffer)))
>> +        WARN("Failed to get first buffer, sample %p\n", sample->private);
>> +    else
>> +    {
>> +        /* release lock and ref taken in mf_create_wg_sample */
>> +        IMFMediaBuffer_Unlock(media_buffer);
>> +        IMFMediaBuffer_SetCurrentLength(media_buffer, sample->size);
>> +        IMFMediaBuffer_Release(media_buffer);
> 
> 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.


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

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.


>> +    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.


>> +
>> +    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.


> (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?

-- 
Rémi Bernon <rbernon at codeweavers.com>