Handle VLRs as pipeline arguments in writers.las (#2942)

* Make user VLR handling robust.

* Cleanup.

* Remove dead header.

* Make things with with Ext VLRs.

* Add more test.

* One more test.

* Remove unnecessary cast.

* Add a real test for the original problem.

* Cleaner logic.

io/LasVLR.cpp

@@ -34,6 +34,11 @@
 
 #include "LasVLR.hpp"
 
+#include <limits>
+#include <nlohmann/json.hpp>
+
+#include <pdal/util/Utils.hpp>
+
 namespace pdal
 {
 
@@ -57,6 +62,115 @@ ILeStream& operator>>(ILeStream& in, LasVLR& v)
 }
 
 
+std::istream& operator>>(std::istream& in, LasVLR& v)
+{
+    NL::json j;
+    in >> j;
+
+    // Make sure there isn't stuff in the input stream after the JSON
+    // object.
+    char c;
+    do
+    {
+        in >> c;
+    } while (std::isspace(c));
+    if (!in.eof())
+        throw pdal_error("Invalid characters following LAS VLR JSON object.");
+
+    // We forced an EOF above, so clear the error state.
+    in.clear();
+    if (!j.is_object())
+        throw pdal_error("LAS VLR must be specified as a JSON object.");
+
+    std::string description;
+    std::string b64data;
+    std::string userId;
+    double recordId(std::numeric_limits<double>::quiet_NaN());
+    for (auto& el : j.items())
+    {
+        if (el.key() == "description")
+        {
+            if (!el.value().is_string()) 
+                throw pdal_error("LAS VLR description must be specified "
+                    "as a string.");
+            description = el.value().get<std::string>();
+            if (description.size() > 32)
+                throw pdal_error("LAS VLR description must be 32 characters "
+                    "or less.");
+        }
+        else if (el.key() == "record_id")
+        {
+            if (!el.value().is_number())
+                throw pdal_error("LAS VLR record ID must be specified as "
+                    "a number.");
+            recordId = el.value().get<double>();
+            if (recordId < 0 ||
+                recordId > (std::numeric_limits<uint16_t>::max)() ||
+                recordId != (uint16_t)recordId)
+                throw pdal_error("LAS VLR record ID must be an non-negative "
+                    "integer less than 65536.");
+        }
+        else if (el.key() == "user_id")
+        {
+            if (!el.value().is_string()) 
+                throw pdal_error("LAS VLR user ID must be specified "
+                    "as a string.");
+            userId = el.value().get<std::string>();
+            if (userId.size() > 16)
+                throw pdal_error("LAS VLR user ID must be 16 characters "
+                    "or less.");
+        }
+        else if (el.key() == "data")
+        {
+            if (!el.value().is_string())
+                throw pdal_error("LAS VLR data must be specified as "
+                    "a base64-encoded string.");
+            b64data = el.value().get<std::string>();
+        }
+        else
+            throw pdal_error("Invalid key '" + el.key() + "' in VLR "
+                "specificiation.");
+    }
+    if (b64data.empty())
+        throw pdal_error("LAS VLR must contain 'data' member.");
+    if (userId.empty())
+        throw pdal_error("LAS VLR must contain 'user_id' member.");
+    if (std::isnan(recordId))
+        recordId = 1;
+
+    v.m_userId = userId;
+    v.m_recordId = recordId;
+    v.m_description = description;
+    v.m_data = Utils::base64_decode(b64data);
+    return in;
+}
+
+
+std::istream& operator>>(std::istream& in, ExtLasVLR& v)
+{
+    return (in >> (LasVLR&)v);
+}
+
+
+std::ostream& operator<<(std::ostream& out, const LasVLR& v)
+{
+    const unsigned char *d(reinterpret_cast<const unsigned char *>(v.data()));
+
+    out << "{\n";
+    out << "  \"description\": \"" << v.description() << "\",\n";
+    out << "  \"record_id\": " << v.recordId() << ",\n";
+    out << "  \"user_id\": \"" << v.userId() << "\",\n";
+    out << "  \"data\": \"" <<Utils::base64_encode(d, v.dataLen()) << "\"\n";
+    out << "}\n";
+    return out;
+}
+
+std::ostream& operator<<(std::ostream& out, const ExtLasVLR& v)
+{
+    return (out << (LasVLR&)v);
+}
+
+
 OLeStream& operator<<(OLeStream& out, const LasVLR& v)
 {
     out << v.m_recordSig;
@@ -97,10 +211,10 @@ OLeStream& operator<<(OLeStream& out, const ExtLasVLR& v)
     return out;
 }
 
-    void LasVLR::write(OLeStream& out, uint16_t recordSig)
-    {
-        m_recordSig = recordSig;
-        out << *this;
-    }
+void LasVLR::write(OLeStream& out, uint16_t recordSig)
+{
+    m_recordSig = recordSig;
+    out << *this;
+}
 
 } // namespace pdal

io/LasVLR.hpp

@@ -102,6 +102,8 @@ class PDAL_DLL LasVLR
 
     friend ILeStream& operator>>(ILeStream& in, LasVLR& v);
     friend OLeStream& operator<<(OLeStream& out, const LasVLR& v);
+    friend std::istream& operator>>(std::istream& in, LasVLR& v);
+    friend std::ostream& operator<<(std::ostream& out, const LasVLR& v);
 
 protected:
     std::string m_userId;
@@ -124,6 +126,8 @@ class ExtLasVLR : public LasVLR
     friend ILeStream& operator>>(ILeStream& in, ExtLasVLR& v);
     friend OLeStream& operator<<(OLeStream& out,
         const ExtLasVLR& v);
+    friend std::istream& operator>>(std::istream& in, ExtLasVLR& v);
+    friend std::ostream& operator<<(std::ostream& out, const ExtLasVLR& v);
 };
 
 } // namespace pdal

io/LasWriter.cpp

@@ -56,8 +56,6 @@
 #include <iostream>
 #include <vector>
 
-#include <nlohmann/json.hpp>
-
 #include <pdal/pdal_features.hpp>
 #include <pdal/DimUtil.hpp>
 #include <pdal/PDALUtils.hpp>
@@ -89,8 +87,7 @@ CREATE_STATIC_STAGE(LasWriter, s_info)
 std::string LasWriter::getName() const { return s_info.name; }
 
 LasWriter::LasWriter() : m_compressor(nullptr), m_ostream(NULL),
-    m_compression(LasCompression::None), m_srsCnt(0),
-    m_userVLRs(new NL::json)
+    m_compression(LasCompression::None), m_srsCnt(0)
 {}
 
 
@@ -148,7 +145,7 @@ void LasWriter::addArgs(ProgramArgs& args)
     args.add("offset_x", "X offset", m_offsetX);
     args.add("offset_y", "Y offset", m_offsetY);
     args.add("offset_z", "Z offset", m_offsetZ);
-    args.add("vlrs", "List of VLRs to set", *m_userVLRs);
+    args.add("vlrs", "List of VLRs to set", m_userVLRs);
 }
 
 void LasWriter::initialize()
@@ -236,31 +233,8 @@ void LasWriter::prepared(PointTableRef table)
 // Capture user-specified VLRs
 void LasWriter::addUserVlrs()
 {
-    for (const auto& v : *m_userVLRs)
-    {
-        uint16_t recordId(1);
-        std::string userId("");
-        std::string description("");
-        std::string b64data("");
-        std::string user("");
-        if (!v.contains("user_id"))
-            throw pdal_error("VLR must contain a 'user_id'!");
-        userId = v["user_id"].get<std::string>();
-
-        if (!v.contains("data"))
-            throw pdal_error("VLR must contain a base64-encoded 'data' member");
-        b64data = v["data"].get<std::string>();
-
-        // Record ID should always be no more than 2 bytes.
-        if (v.contains("record_id"))
-            recordId = v["record_id"].get<uint16_t>();
-
-        if (v.contains("description"))
-            description = v["description"].get<std::string>();
-
-        std::vector<uint8_t> data = Utils::base64_decode(b64data);
-        addVlr(userId, recordId, description, data);
-    }
+    for (const auto& v : m_userVLRs)
+        addVlr(v);
 }
 
 
@@ -573,23 +547,24 @@ void LasWriter::addExtraBytesVlr()
 void LasWriter::addVlr(const std::string& userId, uint16_t recordId,
    const std::string& description, std::vector<uint8_t>& data)
 {
-    if (data.size() > LasVLR::MAX_DATA_SIZE)
+    addVlr(ExtLasVLR(userId, recordId, description, data));
+}
+
+/// Add a standard or variable-length VLR depending on the data size.
+/// \param  evlr  VLR to add.
+void LasWriter::addVlr(const ExtLasVLR& evlr)
+{
+    if (evlr.dataLen() > LasVLR::MAX_DATA_SIZE)
     {
         if (m_lasHeader.versionAtLeast(1, 4))
-        {
-            ExtLasVLR evlr(userId, recordId, description, data);
             m_eVlrs.push_back(std::move(evlr));
-        }
         else
             throwError("Can't write VLR with user ID/record ID = " +
-                userId + "/" + std::to_string(recordId) +
+                evlr.userId() + "/" + std::to_string(evlr.recordId()) +
                 ".  The data size exceeds the maximum supported.");
     }
     else
-    {
-        LasVLR vlr(userId, recordId, description, data);
-        m_vlrs.push_back(std::move(vlr));
-    }
+        m_vlrs.push_back(std::move(evlr));
 }
 
 /// Delete a VLR from the vlr list.

io/LasWriter.hpp

@@ -36,7 +36,6 @@
 
 #include <pdal/pdal_features.hpp>
 #include <pdal/FlexWriter.hpp>
-#include <pdal/JsonFwd.hpp>
 #include <pdal/Streamable.hpp>
 
 #include "HeaderVal.hpp"
@@ -128,7 +127,7 @@ class PDAL_DLL LasWriter : public FlexWriter, public Streamable
     StringHeaderVal<0> m_offsetZ;
     MetadataNode m_forwardMetadata;
     bool m_writePDALMetadata;
-    std::unique_ptr<NL::json> m_userVLRs;
+    std::vector<ExtLasVLR> m_userVLRs;
     bool m_firstPoint;
 
     virtual void addArgs(ProgramArgs& args);
@@ -171,6 +170,7 @@ class PDAL_DLL LasWriter : public FlexWriter, public Streamable
     void openCompression();
     void addVlr(const std::string& userId, uint16_t recordId,
         const std::string& description, std::vector<uint8_t>& data);
+    void addVlr(const ExtLasVLR& evlr);
     void deleteVlr(const std::string& userId, uint16_t recordId);
     void addGeotiffVlrs();
     bool addWktVlr();

pdal/PipelineReaderJSON.cpp

@@ -349,18 +349,18 @@ Options PipelineReaderJSON::extractOptions(NL::json& node)
             continue;
         }
 
-        if (extractOption(options, name, subnode))
-            continue;
-        else if (subnode.is_array())
+        if (subnode.is_array())
         {
             for (const NL::json& val : subnode)
-                if (!extractOption(options, name, val))
+                if (val.is_object())
+                    options.add(name, val);
+                else if (!extractOption(options, name, val))
                     throw pdal_error("JSON pipeline: Invalid value type for "
                         "option list '" + name + "'.");
         }
         else if (subnode.is_object())
             options.add(name, subnode);
-        else
+        else if (!extractOption(options, name, subnode))
             throw pdal_error("JSON pipeline: Value of stage option '" +
                 name + "' cannot be converted.");
     }

test/data/pipeline/issue2937.json.in

@@ -0,0 +1,21 @@
+[
+    {
+        "type": "writers.las",
+        "filename": "@CMAKE_SOURCE_DIR@/test/temp/issue2937_out.las",
+        "vlrs":
+        [
+            {
+                "description": "A first record",
+                "record_id": 42,
+                "user_id": "hobu",
+                "data": "dGhpcyBpcyBzb21lIHRleHQ="
+            },
+            {
+                "description": "A second record",
+                "record_id": 43,
+                "user_id": "hobu",
+                "data": "dGhpcyAqdtBpcyBzb21lIHRleHQ="
+            }
+        ]
+    }
+]