On 6/10/21 3:17 PM, Giovanni Mascellani wrote:
Hi,
Il 10/06/21 18:12, Derek Lesho ha scritto:
This patch probably warrants a few tests. I would test at least
In line of principle I would agree. The reason I am not submitting any is that Nikolay told me on the chat, as I understand it, that for this kind of patches he prefers tests to be left out-of-tree. Since he is the one who usually reviews my MF patches, I am inclined to meet his requests.
- what happens to ::RequestSample calls when a stream is paused.
As soon as the source is restarted, a corresponding number of MEMediaSample is generated.
I assumed that this means that samples are generated anyway during pause and are later emitted after Start, but as others have noted this is not necessarily true: requests could be queued instead of samples. The two cases differ for example if the Start includes a seek (if requests are stored, then samples are emitted at the freshly seeked location, otherwise they are emitted at the location of pausing). And thinking again about it storing the requests seems more logical than storing the samples. Tomorrow I'll check on Windows to see what's happening there.
Yeah that would be really useful to see. My impression is that, since on Windows the stream has a separate request queue, the simplest implementation would just stop processing requests on the stream queue while the stream is paused. This wouldn't cause problems with the source receiving the start request, as it has its own queue and then upon starting again the stream would run through its saved requests.
- whether MESourcePaused or the MEStreamPaused events come first.
Hmm, does this really make sense? They are emitted on two different queues. Does MF guarantees order of delivery across different queues? Even if one is submitted before the other, the system scheduler might decide to execute in the opposite order, if it wakes up the other thread first. Or is there a stronger guarantee?
You're right, never mind.
- IMFMediaEventQueue_QueueEventParamUnk(stream->event_queue,
MEMediaSample, - &GUID_NULL, S_OK, (IUnknown *)sample); + if (stream->parent_source->state == SOURCE_PAUSED) + { + struct pending_sample *pending = malloc(sizeof(*pending)); + if (!pending) + { + ERR("Cannot allocate pending sample\n"); + goto out; + } + pending->sample = sample; + sample = NULL; + list_add_tail(&stream->pending_samples, &pending->entry);
Since our media source outputs uncompressed samples, if it's common behavior for an application to request a significant amount of samples while the source is paused, we might instead want to add a separate sample request queue. (Lest we use too much memory) I think this is what I was referring to in my old comment.
More than the memory usage (which I doubt will be significant: after all until now apparently even the usage of Pause was not that pressing), I find concerning the observable behavior, as I said above. I will check better.
Thanks for the review, Giovanni.