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

packetPool: ignore TEI and let the caller decide what to do #22

Closed
wants to merge 1 commit into from

Conversation

tmm1
Copy link
Contributor

@tmm1 tmm1 commented Apr 5, 2021

I wasn't sure why TEI marked packets were being dropped altogether.

It makes more sense to me to return the data in-tact to the caller and let them decide what to do. Chances are the audio/video can be decoded even if there are errors, because many codecs has some sort of error concealment built-in. In fact, its likely easier for the decoder to function if all the data is there and some of it is corrupted, vs chunks of data randomly missing in the middle.

wdyt?

@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) to 76.619% when pulling ee5d534 on tmm1:packet-pool-tei into 077af8e on asticode:master.

@barbashov
Copy link
Contributor

I've got something to add here.

I look into ffmpeg sources when in doubt :) Looks like ffmpeg doesn't drop TEI packets by default, it just marks them as corrupt to let the decoder decide.

@asticode
Copy link
Owner

I don't really like the idea of removing this check. What do you think about this:

  • add an optCorruptedPacketHandler to Demuxer of type type CorruptedPacketHandler func(p *Packet) (skip bool)
  • add a DemuxerOptCorruptedPacketHandler that would look like:
func DemuxerOptCorruptedPacketHandler(h CorruptedPacketHandler) func(*Demuxer) {
	return func(d *Demuxer) {
		d.optCorruptedPacketHandler = h
	}
}
  • in packetPool.add, when getting p.Header.TransportErrorIndicator, if the option is set, use it to check whether the packet should be skipped. Skip the packet otherwise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants