media icon indicating copy to clipboard operation
media copied to clipboard

Incorporate MediaSourceEventListener.onLoadStarted with retryCount pa…

Open colinkho opened this issue 1 year ago • 2 comments

…rameter with backward compatibility

colinkho avatar Sep 30 '24 05:09 colinkho

I think in its current form this stops calling any existing implementations of AnalyticsListener.onLoadStarted(EventTime, LoadEventInfo, MediaLoadData) (the one that's now deprecated). I checked this by adding some logging locally to this override in EventLogger and I see the logging before this change, but not after it.

We need to keep these implementations working - because although they're marked as deprecated, deprecated things are still expected to work.

Ensuring both the old and new callbacks continue working, without accidentally multiplying the calls, is somewhat tricky (you mentioned you experimented with putting some duplication in the EventDispatcher implementation, but this results in re-duplicating the events when they are forwarded inside e.g. CompositeMediaSource).

We can put the duplication in the callsites instead (i.e. in each MediaSource implementation). One question then becomes: how do we ensure that any custom MediaSource implementations call the new method (since we can't change them)? You could argue that this doesn't matter, because an app with a custom MediaSource impl also owns their custom AnalyticsListener impl, so they can migrate either both or neither - and therefore remain compatible with themselves.

icbaker avatar Oct 09 '24 11:10 icbaker

I think in its current form this stops calling any existing implementations of AnalyticsListener.onLoadStarted(EventTime, LoadEventInfo, MediaLoadData) (the one that's now deprecated). I checked this by adding some logging locally to this override in EventLogger and I see the logging before this change, but not after it.

We need to keep these implementations working - because although they're marked as deprecated, deprecated things are still expected to work.

Ensuring both the old and new callbacks continue working, without accidentally multiplying the calls, is somewhat tricky (you mentioned you experimented with putting some duplication in the EventDispatcher implementation, but this results in re-duplicating the events when they are forwarded inside e.g. CompositeMediaSource).

We can put the duplication in the callsites instead (i.e. in each MediaSource implementation). One question then becomes: how do we ensure that any custom MediaSource implementations call the new method (since we can't change them)? You could argue that this doesn't matter, because an app with a custom MediaSource impl also owns their custom AnalyticsListener impl, so they can migrate either both or neither - and therefore remain compatible with themselves.

Thanks for the feedback and makes sense. I think duplicating the calls at each callsite makes sense to me since this is treating the updated API as a new API entirely. This seems cleaner too. I will adjust this

colinkho avatar Oct 09 '24 17:10 colinkho