Incorporate MediaSourceEventListener.onLoadStarted with retryCount pa…

[colinkho]

…rameter with backward compatibility

[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.

[icbaker]

you might need to use a different event type here, so we can test that both the new method and the old method are called

[colinkho]

Any naming convention to suggest here? I was thinking EVENT_ON_LOAD_STARTED but not sure if that is confusing and outside of Google's naming conventions

[icbaker]

What about keeping EVENT_LOAD_STARTED for the new un-deprecated one and renaming the old one to EVENT_LOAD_STARTED_WITHOUT_RETRY_COUNT? It's a bit long, but it only needs to live as long as we keep the deprecated method.

[icbaker]

Ah because i was reading this in a test, i thought this constant was defined in the test - my bad, now i see it's defined in AnalyticsListener interface.

In that case i retract my comment, we shouldn't duplicate this constant imo - we should find a different way to test that both the new and old methods are called.

[icbaker]

this call (and the one below in this method) are basically forwarding from an inner MediaSource instance. SImilar to CompositeMediaSource.ForwardingEventListener we should implement both the old and new methods here, and forward each one to the respective EventDispatcher method.

Otherwise a custom MediaSource that hasn't yet switched to dispatching the new method will have all its onLoadStarted events dropped at this point.

[colinkho]

Makes a lot of sense. Will do!

[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.

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