[PATCH 0/5] MR22: winegstreamer: Implement more sample attributes and timestamps.

[Zebediah Figura (she/her)]

On 5/5/22 02:20, Rémi Bernon (@rbernon) wrote:
>>> diff --git a/dlls/winegstreamer/unixlib.h b/dlls/winegstreamer/unixlib.h
>>> index f4e2ea4966b..7a31020fb87 100644
>>> --- a/dlls/winegstreamer/unixlib.h
>>> +++ b/dlls/winegstreamer/unixlib.h
>>> @@ -122,6 +122,7 @@ struct wg_sample
>>>       UINT32 max_size;
>>>       UINT32 size;
>>>       BYTE *data;
>>> +    struct wg_format *format;
>>>   };
>>>   
>>>   struct wg_parser_buffer
>>
>> I was doubtful about introducing this abstraction in the first place,
>> before we've actually implemented zero-copy, and indeed I'm finding it
>> difficult to reason around...
>>
>> Anyway, it's probably not worth restructuring things at this rate, but I
>> have a hard time feeling like "format" belongs here. It's an in-out
>> parameter, and as an "out" parameter it is a property of the buffer, but
>> as an "in" parameter it isn't, really.
> 
> I think it is, in both ways. The format tells what the client believes
> the sample format is (even if it is uninitialized at the time it gives
> it to wg_transform), but it can describe what it expects in term of data
> size and layout for instance. If it doesn't match what we're going to
> give it, we return a format change notification and the new format.

Maybe. It's not an immediately clear design, though, which is why I'm 
inclined to separate it somehow. (But see below anyway...)

> 
> 
>> Actually, for that matter, why do we need to store the wg_format on the
>> PE side? Can't we just store it on the unix side?
>>
>> (Maybe even check it in the chain function instead of in read_data, and
>> store it in a flag on the object with gst_mini_object_set_qdata(), and
>> then that'd remove the need to allocate GstSample objects. I don't
>> remember if there was another impetus for using those?)
> 
> I don't see what benefit it would have, we need to return the
> information about the new format to the client anyway so that it can
> update the properties it exposes to the MF caller.

Er, right, I forgot we still need to store the format anyway, so ignore 
that part :-)

Still, the first part seems reasonable, unless I'm missing something?

Or, frankly, we could make it output-only, and set the "format changed" 
flag entirely on the client side. The way things currently are, the 
logic is kind of split in two, and it feels awkward.

> 
> GstSample seems a much nicer high level API to me, and that it is better
> to use it than than something low level like gst_mini_object_set_qdata.
> 
> In either case you will need to allocate something, either the GstSample
> or the wg_format you keep on the buffers, because the format can change
> and because buffers can be queued. GstSample and caps is much cleaner
> imho.
> 
> In any case I'd rather avoid another complete rewrite at this point,
> it's been months already since I started upstreaming these patches and I
> would prefer to keep things like this if it's not completely backwards.
> 
> 
>>> @@ -427,7 +462,18 @@ NTSTATUS wg_transform_read_data(void *args)
>>>            return STATUS_SUCCESS;
>>>        }
>>>    
>>> -    if ((status = read_transform_output_data(transform->output_buffer, sample)))
>>> +    if (sample->format && (caps = gst_sample_get_caps(transform->output_sample)))
>>> +    {
>>> +        wg_format_from_caps(&format, caps);
>>> +        if (!wg_format_compare(&format, sample->format))
>>> +        {
>>> +            *sample->format = format;
>>> +            params->result = MF_E_TRANSFORM_STREAM_CHANGE;
>>> +            return STATUS_SUCCESS;
>>> +        }
>>> +    }
>>
>> This looks wrong; aren't we dropping the sample data on the floor?
> 
> No? We're not releasing output_sample there.
> 

Sorry, not dropping it on the floor exactly, but we're also not filling 
the buffer, whereas the mfplat code (and the tests) looks like it 
expects the buffer to be filled.