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

Adjust dts warning messages in stream #41467

Merged

Conversation

uvjustin
Copy link
Contributor

@uvjustin uvjustin commented Oct 8, 2020

Proposed change

We added some warning messages when encountering out of order packets in #39844 . It seems like the pyav rtsp demuxer automatically drops out of order packets, but it allows packets with the same DTS through.
For out of order packets, this PR skips printing the warning if consecutive packets with the same DTS are encountered. With this change, we do not expect the out of order packet warning to ever be printed as the demuxer should drop the strictly out of order packets, and this warning should only print for these strictly out of order packets.
This PR also adjusts the overflow warning to include the last dts value for help with troubleshooting.

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Example entry for configuration.yaml:

# Example configuration.yaml

Additional information

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • The code has been formatted using Black (black --fast homeassistant tests)
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

  • No score or internal
  • 🥈 Silver
  • 🥇 Gold
  • 🏆 Platinum

To help with the load of incoming pull requests:

@probot-home-assistant
Copy link

Hey there @hunterjm, mind taking a look at this pull request as its been labeled with an integration (stream) you are listed as a codeowner for? Thanks!
(message by CodeOwnersMention)

@uvjustin uvjustin changed the title Adjust dts warning messages Adjust dts warning messages in stream Oct 8, 2020
@uvjustin
Copy link
Contributor Author

uvjustin commented Oct 8, 2020

@hunterjm You were right about still needing this monotonicity check. It looks like the demuxer does get rid of strictly out of order packets but still allows packets with equal dts through.
As for the overflow, I saw some feedback with people encountering the overflow warning. I thought it would be good to include the last_dts value as well for more insight. Some of these warnings might not be due to a true overflow - we might be getting packets which are coming through the demuxer with incorrect dts values. If this it the case, we might want to wait until we get 2 consecutive packets with the bad dts/large gap before resetting the stream.

@uvjustin uvjustin force-pushed the adjust-dts-warning-messages-in-stream branch from 6a328bc to 9f47c5e Compare October 8, 2020 07:52
@uvjustin uvjustin force-pushed the adjust-dts-warning-messages-in-stream branch from 1856daf to bab5fdb Compare October 10, 2020 17:18
@uvjustin
Copy link
Contributor Author

Just removed the out of packet warning. It's not really necessary anymore.

@uvjustin uvjustin requested a review from hunterjm October 10, 2020 17:18
@hunterjm hunterjm merged commit 1c6d0d1 into home-assistant:dev Oct 11, 2020
weissm pushed a commit to weissm/core that referenced this pull request Oct 16, 2020
* Adjust dts warning messages

* Wait for second packet with overflow dts condition

* Rename dts gap watch variable

* Remove out of order packet warning
@hunterjm hunterjm added this to the 0.116.5 milestone Oct 16, 2020
@Johndolk
Copy link

Sorry in front for a possible total stupid question. I am also getting the "Timestamp overflow ressetting stream" error constantly.
Would really like try adding this fix above you have made - But how the heck do i add this in my Home Assistant?

I am totally new with this so please bear with me :)

@hunterjm
Copy link
Member

@Johndolk - it should be a part of the 0.117 release.

@Johndolk
Copy link

@Johndolk - it should be a part of the 0.117 release.

Ohh i see! So these things are stuff that are supposed to be added in a new update - I am learning new things about this every day but for me it is a bit of a jungle! Thank you so much for replying so swiftly

@uvjustin uvjustin deleted the adjust-dts-warning-messages-in-stream branch November 25, 2020 16:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants