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

Fix pending Metadata timestamp relative to subsample offset #4557

Closed
wants to merge 1 commit into from

Conversation

phhusson
Copy link
Contributor

Since MetadataRenderer queues events until event times are current
position, this leads to metadata event like SCTE-35 not being delivered.

To compare to player timestamp, we need to include subsample offset.

@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

Since MetadataRenderer queues events until event times are current
position, this leads to metadata event like SCTE-35 not being delivered.

To compare to player timestamp, we need to include subsample offset
@phhusson phhusson force-pushed the fix/metadata-timestamp branch from 74180be to 30aef51 Compare July 23, 2018 14:36
@googlebot
Copy link

CLAs look good, thanks!

@ojw28
Copy link
Contributor

ojw28 commented Jul 23, 2018

I don't think this fix is correct. Could you please provide some reproduction steps and sample content?

@phhusson
Copy link
Contributor Author

I'm debugging and reproducing this by adding:

  • if(pendingMetadataCount > 0 && pendingMetadataTimestamps[pendingMetadataIndex] > positionUs) {
  •  android.util.Log.d("SAH", "Delaying metadata sample " + positionUs + " : " + pendingMetadataTimestamps[pendingMetadataIndex]);
    
  • }
    in MetadataRender's render function
    And by adding a MetadataOutput to the player, and listening for events.

On my internal test streams, without this fix, pendingMetadataTimestamps[pendingMetadataIndex] is in the order of 600 000s, and is never received.
With it, few seconds. It is de-queued few seconds after reception, and is received by the MetadataOutput.

I'll search for a public scte-35 stream, or see if I can publish mine.

@ojw28
Copy link
Contributor

ojw28 commented Jul 23, 2018

You can also send your test stream to [email protected]. It's somewhat hard to tell whether the issue is with the media itself, or with ExoPlayer (and if so, where), without being able to take a look. I'm pretty sure we have some SCTE-35 test streams internally that work correctly with the current implementation.

@phhusson
Copy link
Contributor Author

@ojw28
Copy link
Contributor

ojw28 commented Jul 25, 2018

Thanks for the sample! I'm pretty sure the issue is elsewhere, so I'll go ahead and close this pull request. I've filed an issue here.

@ojw28 ojw28 closed this Jul 25, 2018
@google google locked and limited conversation to collaborators Nov 23, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants