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

[Gabriel Ivăncescu]

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.

Thanks,
Gabriel