Fix draft-18 subgroup object framing

Author: mondain

include/openmoq/publisher/transport/moqt_control_messages.h

@@ -149,10 +149,11 @@ std::vector<std::uint8_t> encode_subgroup_header(DraftVersion draft,
                                                  std::uint64_t subgroup_id,
                                                  bool end_of_group);
 
-// Object fields to append to an already-open subgroup stream. Per spec
-// §10.4.2 the first object on the stream carries its absolute Object ID
-// (pass std::nullopt for previous_object_id); subsequent objects encode
-// Object ID Delta = object_id - previous_object_id - 1.
+// Object fields to append to an already-open subgroup stream. The first object
+// on the stream carries its absolute Object ID (pass std::nullopt for
+// previous_object_id); subsequent objects encode Object ID Delta =
+// object_id - previous_object_id - 1. Draft-18 only emits Object Status when
+// the payload length is zero.
 std::vector<std::uint8_t> encode_subgroup_object(DraftVersion draft,
                                                  std::optional<std::uint64_t> previous_object_id,
                                                  std::uint64_t object_id,

src/transport/moqt_control_messages.cpp

@@ -1137,12 +1137,11 @@ std::vector<std::uint8_t> encode_subgroup_object(DraftVersion draft,
         previous_object_id.has_value() ? (object_id - *previous_object_id - 1) : object_id;
     std::vector<std::uint8_t> bytes;
     append_varint(bytes, object_id_delta);
-    if (draft == DraftVersion::kDraft18) {
-        // draft-18 subgroup object encoding includes Object Status. For normal
-        // payload-carrying objects this is OBJECT_STATUS_NORMAL (0).
+    append_varint(bytes, payload.size());
+    if (draft == DraftVersion::kDraft18 && payload.empty()) {
+        // draft-18 only carries Object Status after a zero payload length.
         append_varint(bytes, 0);
     }
-    append_varint(bytes, payload.size());
     bytes.insert(bytes.end(), payload.begin(), payload.end());
     return bytes;
 }

tests/moqt_control_messages_test.cpp

@@ -1,6 +1,8 @@
 #include "openmoq/publisher/transport/moqt_control_messages.h"
 
 #include <cstdint>
+#include <cstddef>
+#include <optional>
 #include <iostream>
 #include <span>
 #include <string>
@@ -20,6 +22,7 @@ using openmoq::publisher::transport::decode_server_setup_message;
 using openmoq::publisher::transport::decode_varint;
 using openmoq::publisher::transport::encode_request_ok_message;
 using openmoq::publisher::transport::encode_setup_message;
+using openmoq::publisher::transport::encode_subgroup_object;
 
 bool expect(bool condition, const std::string& message) {
     if (!condition) {
@@ -152,5 +155,37 @@ int main() {
         ok &= expect(message.request_id == 7, "expected draft-16 request_ok request_id to remain parsed");
     }
 
+    {
+        const std::vector<std::uint8_t> payload = {0xaa, 0xbb, 0xcc};
+        const std::vector<std::uint8_t> bytes =
+            encode_subgroup_object(DraftVersion::kDraft18, std::nullopt, 0, payload);
+        std::size_t offset = 0;
+        std::uint64_t object_id_delta = 99;
+        std::uint64_t payload_length = 99;
+        ok &= expect(decode_varint(bytes, offset, object_id_delta) && object_id_delta == 0,
+                     "expected draft-18 first subgroup object id delta");
+        ok &= expect(decode_varint(bytes, offset, payload_length) && payload_length == payload.size(),
+                     "expected draft-18 payload length immediately after object id delta");
+        ok &= expect(offset + payload_length == bytes.size(), "expected draft-18 payload object to omit status");
+        ok &= expect(std::vector<std::uint8_t>(bytes.begin() + static_cast<std::ptrdiff_t>(offset), bytes.end()) == payload,
+                     "expected draft-18 payload bytes after length");
+    }
+
+    {
+        const std::vector<std::uint8_t> bytes =
+            encode_subgroup_object(DraftVersion::kDraft18, std::nullopt, 0, {});
+        std::size_t offset = 0;
+        std::uint64_t object_id_delta = 99;
+        std::uint64_t payload_length = 99;
+        std::uint64_t object_status = 99;
+        ok &= expect(decode_varint(bytes, offset, object_id_delta) && object_id_delta == 0,
+                     "expected draft-18 empty object id delta");
+        ok &= expect(decode_varint(bytes, offset, payload_length) && payload_length == 0,
+                     "expected draft-18 empty object payload length");
+        ok &= expect(decode_varint(bytes, offset, object_status) && object_status == 0,
+                     "expected draft-18 empty object status after zero length");
+        ok &= expect(offset == bytes.size(), "expected draft-18 empty object to contain no payload");
+    }
+
     return ok ? 0 : 1;
 }