diff --git a/include/AlcPacket.h b/include/AlcPacket.h index 4b0e88b6..c74ead9a 100644 --- a/include/AlcPacket.h +++ b/include/AlcPacket.h @@ -47,7 +47,8 @@ namespace LibFlute { * @param close_object_flag Set the LCT Close Object flag (RFC 3451 clause 5.1, 'B' bit) on this packet */ AlcPacket(uint64_t tsi, uint16_t toi, FecOti fec_oti, const std::vector& symbols, size_t max_size, uint32_t fdt_instance_id, - bool close_session_flag = false, bool close_object_flag = false); + bool close_session_flag = false, bool close_object_flag = false, + bool include_fti = false); /** * Default destructor. diff --git a/include/File.h b/include/File.h index de3a7f95..e566de7d 100644 --- a/include/File.h +++ b/include/File.h @@ -129,6 +129,16 @@ namespace LibFlute { _meta.content_type = fdt_entry.content_type; _meta.content_md5 = fdt_entry.content_md5; _meta.expires = fdt_entry.expires; + // The encoding has to come across too. Reception bootstrapped from EXT_FTI cannot know an + // object is content encoded, because EXT_FTI carries FEC parameters and not that; without + // this the object is delivered still compressed, with a Content-MD5 that will not match. + _meta.content_encoding = fdt_entry.content_encoding; + // And so does the content length. Bootstrapping sets it from EXT_FTI's transfer length, + // which is the same number only for an object carried without a content encoding. For an + // encoded one the FDT holds the decoded length and EXT_FTI the encoded one, so keeping the + // bootstrap value makes the post-decode length check compare against the wrong figure. + // fec_oti is still left alone: its transfer length is what the reassembly is keyed on. + _meta.content_length = fdt_entry.content_length; }; /** diff --git a/include/Transmitter.h b/include/Transmitter.h index 4b532708..082bb2ad 100644 --- a/include/Transmitter.h +++ b/include/Transmitter.h @@ -737,6 +737,9 @@ namespace LibFlute { boost::asio::ip::address _tunnel_local_address; std::atomic _active; + // Which profile's obligations apply to this session. Needed on the send path because it + // decides whether the object's transfer length can travel in the FDT at all. + Profile _profile; std::atomic _deactivate_when_all_files_sent = false; }; diff --git a/src/AlcPacket.cpp b/src/AlcPacket.cpp index a1eda718..7fdaf2e8 100644 --- a/src/AlcPacket.cpp +++ b/src/AlcPacket.cpp @@ -225,7 +225,7 @@ LibFlute::AlcPacket::AlcPacket(char* data, size_t len) } LibFlute::AlcPacket::AlcPacket(uint64_t tsi, uint16_t toi, LibFlute::FecOti fec_oti, const std::vector& symbols, size_t max_encoding_symbol_size, uint32_t fdt_instance_id, - bool close_session_flag, bool close_object_flag) + bool close_session_flag, bool close_object_flag, bool include_fti) : _fec_oti(fec_oti) { // TSI width: this wire scheme always carries a 16-bit half-word component (half_word_flag=1, @@ -243,8 +243,10 @@ LibFlute::AlcPacket::AlcPacket(uint64_t tsi, uint16_t toi, LibFlute::FecOti fec_ if (wide_tsi) { lct_header_len += 1; } - if (toi == 0) { // Add extensions for FDT + if (toi == 0) { // EXT_FDT (one word) plus EXT_FTI (four words) lct_header_len += 5; + } else if (include_fti) { // EXT_FTI only, four words + lct_header_len += 4; } auto max_packet_length = max_encoding_symbol_size + @@ -280,14 +282,27 @@ LibFlute::AlcPacket::AlcPacket(uint64_t tsi, uint16_t toi, LibFlute::FecOti fec_ *((uint16_t*)hdr_ptr) = htons(toi); hdr_ptr += 2; - if (toi == 0) { // Add extensions for FDT + if (toi == 0) { // EXT_FDT describes the FDT instance and belongs only on the FDT itself *((uint8_t*)hdr_ptr) = EXT_FDT; hdr_ptr += 1; *((uint8_t*)hdr_ptr) = 1 << 4 | (fdt_instance_id & 0x000F0000) >> 16; hdr_ptr += 1; *((uint16_t*)hdr_ptr) = htons(fdt_instance_id & 0x0000FFFF); hdr_ptr += 2; + } + /* EXT_FTI goes on the FDT always, and on a content object when asked for. The caller asks when + the FDT cannot carry the object's transfer length, which under the MBMS Download Profile is any + content-encoded object: TS 26.346 V18.2.0 clause L.4.4 forbids Transfer-Length in the FDT and + RFC 3926 clause 3.4.2 only lets Content-Length stand in for an unencoded object, so in band is + the only route left and RFC 3926 clause 5 obliges every receiver to support it. + + This is a deliberate departure from a "should", taken because the alternative departs from a + "shall not". TS 26.346 V18.2.0 clause L.4.7: "FEC Object Transmission Information in FLUTE + packets which carry symbols of content files should be conveyed by the FEC-OTI parameters in + the FDT". The FDT still carries every FEC-OTI parameter the profile permits; what it cannot + carry, and what travels here instead, is the transfer length alone. */ + if (toi == 0 || include_fti) { *((uint8_t*)hdr_ptr) = EXT_FTI; hdr_ptr += 1; *((uint8_t*)hdr_ptr) = 4; // HEL diff --git a/src/FileDeliveryTable.cpp b/src/FileDeliveryTable.cpp index 0c90c56e..15040df8 100644 --- a/src/FileDeliveryTable.cpp +++ b/src/FileDeliveryTable.cpp @@ -551,8 +551,10 @@ auto LibFlute::FileDeliveryTable::to_string() const -> std::string { The parser below is deliberately unchanged, because the same clause's NOTE keeps this one mandatory for receivers: "With the exception of Transfer-Length, which is mandatory, these - parameters are optional to support by the FLUTE receiver." Nothing is lost on the wire either: - the receive path falls back to Content-Length when the attribute is absent. */ + parameters are optional to support by the FLUTE receiver." For an object carried without a + content encoding nothing is lost, because RFC 3926 clause 3.4.2 lets Content-Length stand in. + For a content-encoded object the length is genuinely absent from the FDT, and the sender + supplies it in the object's own EXT_FTI instead; see the parser above. */ const bool mbms_download_profile = (_profile == Profile::Mbms3gpp); if (!mbms_download_profile && file.fec_oti.transfer_length) f->SetAttribute("Transfer-Length", file.fec_oti.transfer_length); diff --git a/src/Receiver.cpp b/src/Receiver.cpp index 26bbc261..da4c7cbf 100644 --- a/src/Receiver.cpp +++ b/src/Receiver.cpp @@ -356,6 +356,19 @@ auto LibFlute::Receiver::process_alc_datagram(char* data, size_t bytes_recvd) -> // the FDT once it arrives (see the merge below) or left blank if it never does. FileDeliveryTable::FileEntry fe{static_cast(alc.toi()), "", static_cast(alc.fec_oti().transfer_length), "", "", 0, alc.fec_oti()}; _files[alc.toi()] = std::make_shared(fe); + + /* The FDT may already have described this object and been unable to say how long it is, + in which case its entry is waiting here rather than lost: adopt it now, so the object + is written to its Content-Location and decoded per its Content-Encoding instead of + landing anonymous and still compressed. */ + if (_fdt) { + for (const auto& entry : _fdt->file_entries()) { + if (entry.toi == alc.toi()) { + _files[alc.toi()]->adopt_fdt_metadata(entry); + break; + } + } + } } if (_files.find(alc.toi()) != _files.end() && !_files[alc.toi()]->complete()) { @@ -427,6 +440,18 @@ auto LibFlute::Receiver::process_alc_datagram(char* data, size_t bytes_recvd) -> existing_file = _files.end(); } if (existing_file == _files.end()) { + if (file_entry.fec_oti.transfer_length == 0) { + /* The FDT does not say how long this object is on the wire, which happens for a + content-encoded object under the MBMS Download Profile: TS 26.346 V18.2.0 + clause L.4.4 forbids the sender from carrying Transfer-Length, and RFC 3926 + clause 3.4.2 only lets Content-Length stand in when no encoding was applied. + Starting reception now would mean partitioning the object to a length that is + simply unknown. Wait instead: the object's own EXT_FTI carries the length, and + the branch above picks this entry's metadata up again once it arrives. */ + spdlog::debug("Deferring reception for TOI {}: transfer length not in the FDT, " + "awaiting the object's EXT_FTI", file_entry.toi); + continue; + } spdlog::debug("Starting reception for file with TOI {}: {} ({})", file_entry.toi, file_entry.content_location, file_entry.content_type); _files.emplace(file_entry.toi, std::make_shared(file_entry)); diff --git a/src/Transmitter.cpp b/src/Transmitter.cpp index 06b976bc..4e7e9b65 100644 --- a/src/Transmitter.cpp +++ b/src/Transmitter.cpp @@ -502,6 +502,7 @@ Transmitter::Transmitter ( const std::string& destination_address, short port, , _tunnel_endpoint(tunnel_endpoint) , _tunnel_local_address() , _active(active) + , _profile(profile) { if (source_address) { _source_address = boost::asio::ip::make_address(source_address.value()); @@ -800,8 +801,17 @@ auto Transmitter::send_next_packet() -> void for(const auto& symbol : symbols) { spdlog::debug("sending TOI {} SBN {} ID {}", file->meta().toi, symbol.source_block_number(), symbol.id() ); } + /* A content-encoded object under the MBMS Download Profile has no way to state its transfer + length in the FDT: TS 26.346 V18.2.0 clause L.4.4 forbids Transfer-Length there, and + RFC 3926 clause 3.4.2 only lets Content-Length stand in when no encoding was applied. Carry + it in the object's own EXT_FTI instead, which every receiver must support. See AlcPacket for + why this is the route taken. */ + const bool fti_on_content_packet = file->meta().toi != 0 && + _profile == Profile::Mbms3gpp && + !file->meta().content_encoding.empty(); auto packet = std::make_shared(_tsi, file->meta().toi, file->meta().fec_oti, symbols, _max_payload, file->fdt_instance_id(), - _session_closing, _closing_objects.count(file->meta().toi) > 0); + _session_closing, _closing_objects.count(file->meta().toi) > 0, + fti_on_content_packet); bytes_queued += packet->size(); boost::asio::ip::udp::endpoint send_endpoint; diff --git a/tests/test_protocol_fixes.cpp b/tests/test_protocol_fixes.cpp index daef70cd..372baab5 100644 --- a/tests/test_protocol_fixes.cpp +++ b/tests/test_protocol_fixes.cpp @@ -392,3 +392,48 @@ TEST(ExpiryAttributesTest, CacheControlStillCarriesOnlyOneChoiceMember) { EXPECT_NE(out.find("no-cache"), std::string::npos); EXPECT_EQ(out.find(">2222<"), std::string::npos); } + +// Whether Content-Length may stand in for a missing Transfer-Length depends on +// whether the object was content encoded, and on nothing else. +// RFC 3926 clause 3.4.2: "If the file is not content encoded before transport +// (and thus the "Content-Encoding" attribute is not used) then the transfer +// length is the length of the original file, and in this case the +// "Content-Length" is also the transfer length." +namespace { +std::string fdt_with(const std::string& file_attrs) { + return std::string("" + "" + ""; +} + +uint64_t parsed_transfer_length(const std::string& file_attrs) { + auto xml = fdt_with(file_attrs); + std::vector buf(xml.begin(), xml.end()); + LibFlute::FileDeliveryTable fdt(1, buf.data(), buf.size()); + for (const auto& e : fdt.file_entries()) { + if (e.toi == 1) return e.fec_oti.transfer_length; + } + throw std::runtime_error("no entry parsed"); +} +} // namespace + +TEST(EncodedObjectTransferLengthTest, ContentLengthStandsInOnlyWithoutAnEncoding) { + // No encoding: the clause authorises the substitution. + EXPECT_EQ(parsed_transfer_length("Content-Length=\"5000\""), 5000u); +} + +TEST(EncodedObjectTransferLengthTest, AnEncodedObjectDoesNotBorrowContentLength) { + // With an encoding the two lengths differ, so borrowing Content-Length would + // hand the decoder a length wrong by however much the encoding changed. The + // length is left unknown for the object's own EXT_FTI to supply. + EXPECT_EQ(parsed_transfer_length("Content-Length=\"5000\" Content-Encoding=\"gzip\""), 0u) + << "an encoded object's transfer length is not its Content-Length"; +} + +TEST(EncodedObjectTransferLengthTest, AnExplicitTransferLengthAlwaysWins) { + EXPECT_EQ(parsed_transfer_length("Content-Length=\"5000\" Transfer-Length=\"4096\""), 4096u); + EXPECT_EQ(parsed_transfer_length( + "Content-Length=\"5000\" Transfer-Length=\"4096\" Content-Encoding=\"gzip\""), 4096u); +}