-
Notifications
You must be signed in to change notification settings - Fork 51
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
Add muxer #16
Add muxer #16
Conversation
Thanks! Cleaned that up |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few remarks regarding last changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In all honesty, this PR is really awesome.
muxer.go
Outdated
return nil | ||
} | ||
|
||
func (m *Muxer) WritePayload(pid uint16, af *PacketAdaptationField, ph *PESHeader, payload []byte) (int, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the public API should be func (m *Muxer) WriteData(af *PacketAdaptationField, d *Data) (int, error)
instead. I don't mind implementing it for PES only for now. *Data
would be sufficient to pass pid
, ph
and payload
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I gotta remember why I haven't done that in the first place, will try to remember in next couple of days and will get back to you :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aha! I remember now why I haven't done it that way.
Data
hasFirstPacket
withPID
in its header. At the same timeData
hasPID
as a separate member. While it's perfectly okay and handy for demuxer API user, that is confusing for muxer API userFirstPacket
has a bunch of fields which have no effect setting inWriteData
case - I will rewrite CC, AF/payload indicators, etc. That could confuse API user as well.
So the main point here is: I've designed WritePayload
to require arguments that really make sense here and are really used.
I get your point as well - it's not consistent with demuxer currently. And it's not especially pretty :) Though I don't really know a way to make it perfect now - seems like we're choosing between two tradeoffs (well, a position I constantly find myself in :))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also I think muxer logic is a bit different by its nature.
Demuxer usage is like "tell me everything you can parsing a TS stream".
While muxer usage is more like "okay, I got a bunch of elementary streams, now let's write payloads. And handle all the low level stuff like PAT/PMT for me, I don't want to be bothered by that".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love your explanation and I get your point about FirstPacket
, that would indeed be confusing.
What about renaming the current Data
to DemuxerData
and creating :
type MuxerData struct {
AdaptationField *PacketAdaptationField
PES *PESData
PID uint16
}
I think that would be a good compromise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me think about it a little bit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well I've done it that way and I like it.
Though don't rush to resolve this thread just yet. I'd like to do some additional testing today to make sure I haven't broken anything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've tested new WriteData
with my project I'm working on. All is ok, no issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! I'll merge the PR then
@asticode can you please resolve threads I've already addressed? I'm starting to get confused as there's a lot of them :) |
Done. Be careful as there are open threads that Github hides in the middle. You have to load them manually. |
Thanks. Yeap, I'd found that hidden threads yesterday. Quite controversial UX decision if you ask me :) |
I've resolved threads that are now fixed. Only 3 open threads remaining! |
As I said previously, it was a very nice PR ❤️ Thanks for sticking with it. I've created a Cheers |
Whoa. Finally after two months, 4k lines of code and 57 commits it happened. Will open a bottle of wine for that on Friday :) Thanks for the throughout review and nice feedback! |
Haha hope it was a good one! It was a pleasure too, let's connect on LinkedIn as well (I've sent you an invite). Cheers |
Congrats! Thanks for all the great work here! 🎉 |
I've created a MPEG-TS muxer based on your demuxer library, as requested in #10 .
I must say here that I had no intention for muxer to support everything demuxer supports. What I'm trying to do is to solve my tasks with the library (I guess that's how open source works anyway). So it's not full-featured, but it still helps a lot.
TODOs and limitations:
Additionally while working on the muxer I added some little things here and there, most notably:
autoDetectPacketSize
now knows how to work withbufio.Reader
correctlypacketPool.add
got an optionflushOnPIDChange
to overcome behaviour when one-packet tables doesn't get returned to demuxer at once.Sorry for the big pull request, but I don't see a way to do it in smaller parts :)