tlv8: unmarshal (silent) bug

[oliverpool]

Servus,

as previously mentioned, I have been playing with hap as well and used your library to test the correctness of my implementation.

While trying to debug my tlv8 unmarshal implementation, I found a bug in the tlv8.Unmarshal function. Please find below a test which marshals a struct, unmarshals it and compare the initial and final structs.

func TestMarhsalUnmarshalDefaultVideoStreamConfiguration(t *testing.T) {
	want := rtp.DefaultVideoStreamConfiguration()
	buf, err := Marshal(want)
	if err != nil {
		t.Fatal(err)
	}

	var is rtp.VideoStreamConfiguration
	err = Unmarshal(buf, &is)
	if err != nil {
		t.Fatal(err)
	}

	if reflect.DeepEqual(is, want) == false {
		t.Fatalf("is=%+v want=%+v", is, want)
	}
}

I get the following error (as you can see the Parameters, Levels and other attributes differ):

is={Codecs:[{Type:0 Parameters:{Profiles:[] Levels:[] Packetizations:[]} Attributes:[{Width:1920 Height:1080 Framerate:30} {Width:1280 Height:720 Framerate:30} {Width:640 Height:360 Framerate:30} {Width:480 Height:270 Framerate:30} {Width:320 Height:180 Framerate:30} {Width:1280 Height:960 Framerate:30} {Width:1024 Height:768 Framerate:30} {Width:640 Height:480 Framerate:30} {Width:480 Height:360 Framerate:30} {Width:320 Height:240 Framerate:15}]}]}

want={Codecs:[{Type:0 Parameters:{Profiles:[{Id:0} {Id:1} {Id:2}] Levels:[{Level:0} {Level:1} {Level:2}] Packetizations:[{Mode:0}]} Attributes:[{Width:1920 Height:1080 Framerate:30} {Width:1280 Height:720 Framerate:30} {Width:640 Height:360 Framerate:30} {Width:480 Height:270 Framerate:30} {Width:320 Height:180 Framerate:30} {Width:1280 Height:960 Framerate:30} {Width:1024 Height:768 Framerate:30} {Width:640 Height:480 Framerate:30} {Width:480 Height:360 Framerate:30} {Width:320 Height:240 Framerate:15}]}]}

---

FYI, my tlv8 implementation is split in 2 steps:

1. construct an (un)marshaller based exclusively on the reflect.Type (a nested tree of func(io.Reader, reflect.Value) error)
2. call this (un)marshaller with the reflect.Value

This way, the first step can be cached based on the type and the whole process is a bit cleaner and faster.

---

I will never repeat it enough: huge thanks for your amazing work on this library!

[brutella]

I've added a fix in branch tlv8-unmarshal-bug. Does this resolve your issue?

[oliverpool]

tlv8-unmarshal-bug solves the issue for unmarshalling tlv8 data marshaled with hap :+1:

---

There is still one incompatibility, since my library does not add an empty 0-tag to separate list items, which hap.Unmarshal expects wile unmarshalling (but the specification is not clear at all, how lists should be encoded, only the section 5.12 mentions separator item).

		type codec struct {
			Attributes []struct {
				Width     uint16 `tlv8:"7"`
				Height    uint16 `tlv8:"8"`
				Framerate byte   `tlv8:"9"`
			} `tlv8:"3"`
		}
		conf := codec{
			Attributes: []struct {
				Width     uint16 `tlv8:"7"`
				Height    uint16 `tlv8:"8"`
				Framerate byte   `tlv8:"9"`
			}{
				{1920, 1080, 30},
				{320, 240, 15},
			},
		}
// gohmbrige: [3 11 7 2 128 7 8 2 56 4 9 1 30     3 11 7 2 64 1 8 2 240 0 9 1 15]
// hap:       [3 11 7 2 128 7 8 2 56 4 9 1 30 0 0 3 11 7 2 64 1 8 2 240 0 9 1 15]

When unmarshalled, my library detects that the tag 3 gets repeated and appends a new element.

hap will discard everything after the first element (Attributes will only contain {Width: 1920, Height: 1080, Framerate: 30}).

Anyway, since this is an unspecified behavior, this is not really a bug.

[brutella]

You're right, the separator item is not defined. But as you've pointed out, section 5.12 mentions kTLVType_Separator as type 0xFF. So this means that as separator item should actually be 0xFF 0x00 and not 0x00 0x00! We might be both wrong. 😄

[oliverpool]

Actually in 5.12 Pairing the separator is 0xFF but in table 9-24 Supported RTP Configuration the delimiter is 0x00 :roll_eyes:

---

14.1.1

There may be multiple, separate TLV items of the same type if separated by a TLV item of a different type.

So the separator (or delimiter) is likely needed only within list containing only simple types (so actually useless in the case of pairing :laughing: )