Re: [PATCH v2 1/2] quartz: Cache IMediaSeeking for filters.

[Zebediah Figura]

On 4/7/20 9:17 AM, Gabriel Ivăncescu wrote:
> On 07/04/2020 16:47, Nikolay Sivov wrote:
> > On 4/7/20 4:05 PM, Gabriel Ivăncescu wrote:
> > > -static BOOL is_renderer(IBaseFilter *filter)
> > > +static BOOL is_renderer(struct filter *filter)
> > >   {
> > >       IAMFilterMiscFlags *flags;
> > > -    IMediaSeeking *seeking;
> > >       BOOL ret = FALSE;
> > > -    if (SUCCEEDED(IBaseFilter_QueryInterface(filter, 
> > > &IID_IAMFilterMiscFlags, (void **)&flags)))
> > > +    if (SUCCEEDED(IBaseFilter_QueryInterface(filter->filter, 
> > > &IID_IAMFilterMiscFlags, (void **)&flags)))
> > >       {
> > >           if (IAMFilterMiscFlags_GetMiscFlags(flags) & 
> > > AM_FILTER_MISC_FLAGS_IS_RENDERER)
> > >               ret = TRUE;
> > >           IAMFilterMiscFlags_Release(flags);
> > >       }
> > > -    else if (SUCCEEDED(IBaseFilter_QueryInterface(filter, 
> > > &IID_IMediaSeeking, (void **)&seeking)))
> > > -    {
> > > -        IMediaSeeking_Release(seeking);
> > > -        if (!has_output_pins(filter))
> > > -            ret = TRUE;
> > > -    }
> > > +    else if (get_filter_seeking(filter) && 
> > > !has_output_pins(filter->filter))
> > > +        ret = TRUE;
> > >       return ret;
> > >   }
> > > @@ -675,12 +680,13 @@ static HRESULT WINAPI 
> > > FilterGraph2_AddFilter(IFilterGraph2 *iface,
> > >       }
> > >       IBaseFilter_AddRef(entry->filter = filter);
> > > +    entry->seeking = NULL;
> > >       list_add_head(&graph->filters, &entry->entry);
> > >       list_add_head(&graph->sorted_filters, &entry->sorted_entry);
> > >       entry->sorting = FALSE;
> > >       ++graph->version;
> > > -    if (is_renderer(filter))
> > > +    if (is_renderer(entry))
> > >           ++graph->nRenderers;
> >
> > Helper name does not imply important state change, will it work if you 
> > queried once right after IBaseFilter_AddRef()?  This looks like the only 
> > place when
> > filter is added to the list. After that you can simply make 
> > is_renderer() check for null pointer.
>
> Hi Nikolay,
>
> I guess it would work for this particular case, but it would be 
> different from how Windows does it, so I'd rather not. I don't think the 
> state change is concerning, because it should be transparent, the rest 
> of the code shouldn't have to be cluttered with such corner case IMO. I 
> believe it should be enough that I commented on the helper why it's done 
> the way it is (plus the tests on next patch).
>
> Note that if I do it on AddRef, there's a possibility of adding 
> side-effects if the MediaSeeking would otherwise not be used at all in 
> another app, for example. In this case it would be queried and 
> eventually released, which might introduce other bugs (because 
> currently, it isn't, and neither on Windows). I personally don't think 
> it's worth it.

Broadly I don't think it's worth spending effort trying to match
Windows' query patterns exactly, even if applications do occasionally
mess up COM in some very creative ways. If we do, we'd want at least a
comment or tests explaining why we're not using simpler code. Note also
that this patch isn't doing a whole lot, since is_renderer() is called
in AddFilter() and thus IMediaSeeking will end up being queried anyway
for every filter that doesn't expose IAMFilterMiscFlags.
> Thanks,
> Gabriel