Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extend EventTime with current window index and update EventLogger to show consistent output #7332

Closed
kgrevehagen opened this issue May 5, 2020 · 7 comments
Assignees

Comments

@kgrevehagen
Copy link

[REQUIRED] Issue description

When transitioning from one MediaSource to another in a ConcatenatingMediaSource, onDecoderInputFormatChanged for the second MediaSource is called before onPositionDiscontinuity. This results in that the eventTime.currentPlaybackPositionMs for the second MediaSource is set to the duration of the first MediaSource, so we get two calls to onDecoderInputFormatChanged for the first MediaSource and none for the second, making it hard for us to do proper analytics.

[REQUIRED] Reproduction steps

Using the ExoPlayer demo app, play the playlist Cats -> Dogs and let the first MediaSource play to the end so the second starts. Then, you can see by the EventLogger that the mediaPos for the second MediaSource has the duration of the first MediaSource set. You can also see that the onPositionDiscontinuity is called after all of this has happened.

D/EventLogger: decoderInputFormat [eventTime=1.06, mediaPos=0.00, window=0, period=0, video, id=2, mimeType=video/avc, res=480x360, fps=30.0]
D/EventLogger: decoderInputFormat [eventTime=1.09, mediaPos=0.00, window=0, period=0, audio, id=1, mimeType=audio/mp4a-latm, channels=2, sample_rate=44100, language=und]


D/EventLogger: decoderInputFormat [eventTime=25.83, mediaPos=24.46, window=1, period=1, video, id=1, mimeType=video/avc, res=1280x720]
D/EventLogger: decoderInputFormat [eventTime=25.83, mediaPos=24.46, window=1, period=1, audio, id=2, mimeType=audio/mp4a-latm, channels=2, sample_rate=44100, language=und]

D/EventLogger: positionDiscontinuity [eventTime=26.39, mediaPos=0.03, window=1, period=1, PERIOD_TRANSITION]

[REQUIRED] Link to test content

Included in the demo app

[REQUIRED] A full bug report captured from the device

N/A

[REQUIRED] Version of ExoPlayer being used

2.11.3

[REQUIRED] Device(s) and version(s) of Android being used

  • Android Emulator with Pixel 3 Api 28
  • Pixel 3 device with Android 10
@kgrevehagen kgrevehagen changed the title onDecoderInputFormatChanged called before onPositionDiscontinuity when transitioning in ConcatenatingMediaSource onDecoderInputFormatChanged called before onPositionDiscontinuity when transitioning in ConcatenatingMediaSource May 5, 2020
@tonihei tonihei self-assigned this May 6, 2020
@tonihei
Copy link
Collaborator

tonihei commented May 6, 2020

This is actually working as intended. The decoder input format is changed before the transition because the decoder starts reading the new file before the first finished playing. This is all needed to ensure a smooth gapless transition.

As you can see in the EventLogger output, the events are still correctly associated with the right item in the playlist ("window=1" for the decoder input format change), allowing you to understand them correctly by just listening to the events.

As an aside, we added an PlaybackStatsListener in 2.11. that provides you with analytics summaries for each item in a playlist, already solving some of the tricky event-to-item association problems.

@kgrevehagen
Copy link
Author

Thanks for the quick reply!

Ok, I understand, that makes sense.
I'm not getting an updated windowIndex in my app, but I must be doing something wrong, so I will have to figure that out.

But, it seems we're getting a mix of information as to when this happened and where(in the timeline) this happened.
The only information we're getting for the format change is the window(and period) and mediaPos. But the mediaPos is not for the window we got the format change.
In the example above, we get mediaPos=24.46 and window=1, but what information does that give me?
What information I mean I should be getting:
WHEN did this occur: mediaPos=24.46 and window=0. This is the state of the timeline at the time when the format change actually happened.
WHERE did this occur: mediaPos=0.00 and window=1. This is for what mediaPos and window this format change actually happened.
A bit hard to explain, but can you see the difference?

Because if item nr 2 is only 10 seconds long, there really doesn't exist any mediaPos=24.46 and window=1.
Is it still intended to be like this and/or might it be missing something? Or is there some other way I can calculate the WHERE?

I'll look into the PlaybackStatsListener to see if that can help me on the way.

Thanks again.

@tonihei
Copy link
Collaborator

tonihei commented May 8, 2020

EventTime contains both the WHEN and the WHERE as you called it.

  • WHEN = EventTime.currentPlaybackPositionMs + Player.getCurrentWindowIndex()
  • WHERE = EventTime.eventPlaybackPositionMs + EventTime.windowIndex

But I see there is room for improvement here:

  1. EventLogger shouldn't mix the two together because it may be confusing to have the WHERE window index and the WHEN media time. This should probably change to the WHERE media time (or both, if different).
  2. The WHEN window index (and period data) is only available through the player interface. This is not ideal given all the other information is already in EventTime. We should just add it in there as well for consistency.

Marking as an enhancement to change these two things.

@tonihei tonihei changed the title onDecoderInputFormatChanged called before onPositionDiscontinuity when transitioning in ConcatenatingMediaSource Extend EventTime with current window index and update EventLogger to show consistent output May 8, 2020
@kgrevehagen
Copy link
Author

kgrevehagen commented May 11, 2020

Thank you, that worked perfectly! I agree to the two improvements, makes sense, and looking forward to it :)

Regarding my other issue where I was not getting an updated windowIndex, this seems to be caused by my custom MediaSource. I'm using a custom MediaSource to resolve the urls, because I only have an id to begin with. So I need to do an api call in order to decide what media source it should use(progressive, dash, hls etc).
My custom MediaSource extended BaseMediaSource, and that was giving me this weird behavior. Simply changing it to extend CompositeMediaSource(and fixing some small build errors etc) instead fixed it. Do you think this is a bug in ExoPlayer(which I should create a ticket for) or did I use it wrongly?

@tonihei
Copy link
Collaborator

tonihei commented May 12, 2020

Do you think this is a bug in ExoPlayer(which I should create a ticket for) or did I use it wrongly?

That's impossible to say without knowing what went wrong. We generally advise to use CompositeMediaSource to ensure it's done correctly, but there is nothing wrong with using BaseMediaSource directly. You could compare your BaseMediaSource implementation with all the additional code added by using CompositeMediaSource to see where the difference is. If the difference is something that is undocumented and you couldn't have implemented yourself, please file a new issue so we can improve that.

ojw28 pushed a commit that referenced this issue May 13, 2020
We currently use the playback position in the current window, not the
position in the window the event belongs to. This creates confusing
outputs, e.g, window=1, mediaPos=100, even if the position refers to
window 0.

issue:#7332
PiperOrigin-RevId: 310896908
ojw28 pushed a commit that referenced this issue May 14, 2020
We currently use the playback position in the current window, not the
position in the window the event belongs to. This creates confusing
outputs, e.g, window=1, mediaPos=100, even if the position refers to
window 0.

issue:#7332
PiperOrigin-RevId: 310896908
andrewlewis pushed a commit that referenced this issue May 18, 2020
EventTime contains information about when an event happened and where
it belongs to. Both places can be fully described using timeline, window
index, media period id and position.

Right now, only the information for where the event belongs to is fully
contained in EventTime, whereas the time when the event happened only has
the position, and none of the other information (timeline, window, period).

This change adds the missing information, so that the EventTime can easily
be used without having access to the Player. This also ensures Event
metadata is self-contained and can be stored and reused later.

issue:#7332
PiperOrigin-RevId: 311727004
@ojw28
Copy link
Contributor

ojw28 commented Jun 11, 2020

@tonihei - Can this be closed?

@tonihei
Copy link
Collaborator

tonihei commented Jun 11, 2020

Yes, the enhancement is implemented and the EventLogger updated with the commits above.

@tonihei tonihei closed this as completed Jun 11, 2020
@google google locked and limited conversation to collaborators Aug 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants