Packet switch to receivers, packet modification #92

alextarasov1 · 2018-07-10T16:36:34Z

Gots has the ability to read packets, but its ability to create or modify packets is very limited. Packet creation is an essential feature for writing unit tests.

Packet type can now use receivers. These changes will not break the existing library interfaces and will remain compatible with old versions of the library. This will be more consistent with the rest of the library (EBP, PSI, PES, SCTE35 all use receivers). Packet creation and modification functionality is added. Almost all fields of the packet can now be read, modified, and created. Error checking was added that checks the packet so it is not corrupted, or in an invalid state.

Old way to create packet:

pkt := SetCC(
	Create(
		uint16(13),
		WithHasPayloadFlag,
		WithPUSI),
	7)

New way to create packet:

pkt := NewPacket()
pkt.SetPID(13)
pkt.SetAdaptationFieldControl(PayloadFlag)
pkt.SetPayloadUnitStartIndicator(true)
pkt.SetContinuityCounter(7)

guygrigsby · 2018-07-10T19:34:44Z

packet/adaptationfield.go

+
+// invalid returns true if the size of the slice (AdaptationField) is invalid,
+// or the adaptation field doesnt exist
+func (af AdaptationField) invalid() bool {


Should be valid. Calling !invalid() is awkward.

or isValid. Functions/methods that return boolean values should be name interrogatively.

guygrigsby · 2018-07-10T19:37:10Z

packet/modify_test.go

+		"000000000000000000000000000000000"
+)
+
+func createPacketEmptyBody(t *testing.T, header string) (p Packet) {


What does body mean? If it means "payload" let's call it payload. Body has no specific meaning in MPEGTS.

guygrigsby · 2018-07-10T19:41:19Z

packet/modify_test.go

+	return
+}
+
+func assertPacket(t *testing.T, target Packet, generated Packet) {


This swallows all error context. How do we know what failed?

mikereedell · 2018-07-11T15:16:36Z

packet/adaptationfield.go

+	oldValue := af.getBit(index, mask)
+	if value != oldValue {
+		if value {
+			return 1 // growing


Instead of documenting the return values in-line, write a GoDoc comment for this function that has the return value in it.

kortschak · 2018-08-03T21:50:46Z

@alextarasov1 It looks like you could also remove adaptationfield/create.go and fix #82 here.

kortschak · 2018-08-03T21:51:52Z

packet/adaptationfield.go

+// transportPrivateDataLength returns the length of the transport private data,
+// if there is no transport private data then its length is zero.
+func (af AdaptationField) transportPrivateDataLength() int {
+	if af.hasTransportPrivateData() {


Invert this condition to return early and reduce the mass of text to the right (Go idiom).

kortschak · 2018-08-03T21:52:01Z

packet/adaptationfield.go

+// adaptationExtensionLength returns the length of the adaptation field extension,
+// if there is no adaptation field extension then its length is zero
+func (af AdaptationField) adaptationExtensionLength() int {
+	if af.hasAdaptationFieldExtension() {


kortschak · 2018-08-03T21:52:44Z

packet/adaptationfield.go

+// 0 is returned if the bit is unchanged.
+// this can be used to find if a field is growing or shrinking.
+func (af AdaptationField) bitDelta(index int, mask byte, value bool) int {
+	if value != af.getBit(index, mask) {


alextarasov1 · 2018-08-06T16:17:41Z

@kortschak I won't remove adaptationfield/create.go in this PR (that way the PR is limited to new features only), but it is certainly a possibility in the future.

It is incorrect and untested and will be replaced by methods in Comcast#92.

WillGunther

Thanks for doing all this work! I appreciate all the attention to detail, and comments. This is really close to being ready for merging, but I would like to have input from some of the other maintainers and the open source community on #98. Many of these functions provide duplicate functionality. While I like the receiver functions, there needs to be a discussion on how to treat the older functions in the packet package.

WillGunther · 2018-08-07T15:42:04Z

packet/adaptationfield.go

+SOFTWARE.
+*/
+
+package packet


I see why this file was not put in packet/adapatationfield as that would cause an import cycle, but it does bother me that now two packages have identical functionality with different functions. This definitely means we have to determine the best course of action for #98

WillGunther · 2018-08-07T15:42:50Z

packet/adaptationfield.go

+// index is the byte in the packet where the bit is located
+// mask is the bitmask that has that bit set to one and the rest of the bits set to zero
+// value is the value that the bit will be set to
+func (af AdaptationField) setBit(index int, mask byte, value bool) {


This also feels a bit redundant, but I suppose this makes sense since packet is type [188]byte and adaptationfield is []byte

WillGunther · 2018-08-07T15:45:52Z

packet/adaptationfield.go

+// parseAdaptationField parses the adaptation field that is present in a packet.
+// this function is only used by packet functions.
+func parseAdaptationField(p *Packet) AdaptationField {
+	return AdaptationField(p[:])


This seems superfluous as it is only used in one spot and is just a type change.

WillGunther · 2018-08-07T15:53:24Z

packet/adaptationfield.go

+}
+
+// RandomAccess returns the value of the random access field in the packet.
+func (af AdaptationField) RandomAccess() (bool, error) {


Would it be possible to get some test coverage on RandomAccess and StreamPriority? Splice point and splice countdown also look like they don't get touched.

WillGunther · 2018-08-07T15:57:36Z

packet/adaptationfield.go

+
+// Length returns the length of the adaptation field
+func (af AdaptationField) Length() (int, error) {
+	if len(af) != PacketSize {


Would it make sense to make type AdaptationField [188]byte as well? That way we ditch the error checking? Or I would recommend thinking about making the AdaptationField a slice with variable length that actually represents the length of the field.

I was just thinking a similar thing. If AdaptationField is type AdaptationField Packet, you get all the conversions for free and the type itself becomes a namespacing tool. (Though see overall comment coming soon).

WillGunther · 2018-08-07T16:04:52Z

packet/modify.go

+// FromBytes creates a ts packet from a slice of bytes 188 in length.
+// If the bytes provided have errors or the slice is not 188 in length,
+// then an error vill be returned along with a nill slice.
+func FromBytes(bytes []byte) (pkt Packet, err error) {


This is a bit vague. Perhaps PacketFromBytes or NewPacketFromBytes

This will read as packet.FromBytes(b) in user code. This is good API.

That's a good point. The above comment can be ignored.

kortschak · 2018-08-07T22:11:21Z

While I like the receiver functions, there needs to be a discussion on how to treat the older functions in the packet package.

My thought, FWIW.

One of them should go away, since being able to do things in two trivially similar ways leads to confusion and style wars. The question is about which and why.

Apologies for the Go101, but it helps explain the reasoning.

In Go methods are used for a variety of reasons:

to satisfy interfaces
to namespace functions
to group documentation
to provide fluent API
style and others

In this case the first is not relevant, since there is only one type, the second has merit, as does the third. In many cases for '*Packet the fourth could be an option if the method returned *Packet, but not for *AdaptationField since many of the methods are errorable (there are ways around this, but not without either adding error state to the packet and adaptation field types, or adding additional types that perform these mutations that hold that sate). The fifth is taste.

Functions are used in the place of generics (when taking interface arguments), or for simple stateless mutations (or world mutations).

Separation of functions into distinct packages is often overdone - here it seems that separating packet and adaptation field is one of those times. The benefits that are gained by doing it are achieved by the method namespacing.

Given the number of functions being provided, ISTM that providing the functions as methods is worthwhile for 2 and 3 above and the para above. It did also seem like the "adaptationfield" name was long for a postentially commonly used package.

WillGunther

This looks good. Thanks for all your hard work! I'm going to try to get another approval, and if not we merge early next week anyway.

Feedback has been addressed

WillGunther · 2018-08-14T15:12:53Z

This is a big change! Thanks for the hard work.

It is incorrect and untested and will be replaced by methods in Comcast#92.

alextarasov1 added 6 commits July 6, 2018 14:51

new packet modification functions

ce3bcc3

added tests for new packet functions

d5c3182

added comments and fixed panic for nil packet

648f358

added adaptation field creation

a735a13

adaptation field tests added

0fec124

adaptation field functionality added

1fcce5d

WillGunther requested review from mikereedell and guygrigsby July 10, 2018 22:44

guygrigsby previously requested changes Jul 10, 2018

View reviewed changes

mikereedell requested changes Jul 11, 2018

View reviewed changes

alextarasov1 added 7 commits July 11, 2018 10:28

most packet functions now return errors

ecbe8a0

adaptation field functions now return errors

073983b

added more comments

c52e055

tests no longer use assertPacket. renamed body to payload.

b7410c9

added MIT License comments

8d3ffde

Corrected comments

52edc71

Merge branch 'master' into packet-create

912f4d9

kortschak mentioned this pull request Aug 1, 2018

packet: use [188]byte as Packet type #84

Merged

alextarasov1 and others added 4 commits August 1, 2018 11:31

Added SetPayload and SetAdaptationField, improved errors

df48b84

Merge branch 'master' into packet-create

21ef079

Merge branch 'master' into packet-create

e5b41f7

Changed packet creation functions to use [188]byte.

c085e3c

kortschak mentioned this pull request Aug 3, 2018

packet/adaptationfield: private data length is not safely handled #82

Closed

kortschak reviewed Aug 3, 2018

View reviewed changes

Inverted if conditions

3883128

WillGunther mentioned this pull request Aug 6, 2018

Proposal: Deprecation of non-receiver functions in packet package #98

Open

kortschak added a commit to kortschak/gots that referenced this pull request Aug 7, 2018

packet/adaptationfield: delete SetPrivateData

95c6813

It is incorrect and untested and will be replaced by methods in Comcast#92.

kortschak mentioned this pull request Aug 7, 2018

packet/adaptationfield: delete SetPrivateData #99

Merged

WillGunther requested changes Aug 7, 2018

View reviewed changes

alextarasov1 added 2 commits August 8, 2018 10:19

Changed type of adaptation field from slice to array

8a11e86

improved test coverage

24ebf98

WillGunther approved these changes Aug 9, 2018

View reviewed changes

mikereedell approved these changes Aug 10, 2018

View reviewed changes

WillGunther merged commit 4c3c478 into Comcast:master Aug 14, 2018

kortschak mentioned this pull request Aug 16, 2018

packet: fix construction func return types #103

Merged

kortschak added a commit to kortschak/gots that referenced this pull request Sep 27, 2018

packet/adaptationfield: delete SetPrivateData

9abccb2

It is incorrect and untested and will be replaced by methods in Comcast#92.

kortschak added a commit to kortschak/gots that referenced this pull request Nov 10, 2018

packet/adaptationfield: delete SetPrivateData

6802734

It is incorrect and untested and will be replaced by methods in Comcast#92.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Packet switch to receivers, packet modification #92

Packet switch to receivers, packet modification #92

alextarasov1 commented Jul 10, 2018 •

edited

Loading

guygrigsby Jul 10, 2018

mikereedell Jul 11, 2018

guygrigsby Jul 10, 2018

guygrigsby Jul 10, 2018

mikereedell Jul 11, 2018

kortschak commented Aug 3, 2018

kortschak Aug 3, 2018

kortschak Aug 3, 2018

kortschak Aug 3, 2018

alextarasov1 commented Aug 6, 2018

WillGunther left a comment

WillGunther Aug 7, 2018

WillGunther Aug 7, 2018

WillGunther Aug 7, 2018 •

edited

Loading

WillGunther Aug 7, 2018

WillGunther Aug 7, 2018

kortschak Aug 7, 2018

WillGunther Aug 7, 2018

kortschak Aug 7, 2018

WillGunther Aug 7, 2018

kortschak commented Aug 7, 2018

WillGunther left a comment

WillGunther commented Aug 14, 2018

Packet switch to receivers, packet modification #92

Packet switch to receivers, packet modification #92

Conversation

alextarasov1 commented Jul 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kortschak commented Aug 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alextarasov1 commented Aug 6, 2018

WillGunther left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WillGunther Aug 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kortschak commented Aug 7, 2018

WillGunther left a comment

Choose a reason for hiding this comment

WillGunther commented Aug 14, 2018

alextarasov1 commented Jul 10, 2018 •

edited

Loading

WillGunther Aug 7, 2018 •

edited

Loading