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

[Zebediah Figura (she/her)]

On 5/6/22 02:01, Rémi Bernon (@rbernon) wrote:
>>>> 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.
> 
> There are cases, like when the application calls ProcessMessage with
> BEGIN_STREAMING, where we want to reset the format and receive format
> change notification, again. Having the stream format stored on the MF
> transform side lets us do that by reseting it to its default, and
> trigger the format change again.

Sure, then the question becomes, can we move the entire logic to the 
client side? That would require a separate call to retrieve the type of 
the current sample, given the below (or alternatively some extra queuing 
on the client side, but that seems more complex than necessary), but 
structurally it feels a lot less awkward.

> 
> Call of Duty: Black Ops 3 depends on these notifications.
> 
> I'll try something, but that game, and the others using the H264 have
> been really sensitive to tiny behavior changes.
> 
> 
>>>>> @@ -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.
> 
> The tests check that the returned sample, on format change, is empty,
> which should be the case, and mf_destroy_wg_sample will convert the wg
> sample properties back to the MF side.
> 
> After a stream change you need, and can, call ProcessOutput once more,
> immediately, to get the sample data.
> 

Okay, I missed that. That's a bizarre API design; I don't know why 
they'd send an empty sample instead of no sample at all...