Comment period for Draco extension

[FrankGalligan]

Hello everyone,

We have been working on adding Draco (geometry compression) to glTF 2.0 as an extension. The two major parts that need to be finalized are the glTF 2.0 Draco extension and the Draco bitstream specification. We would like to close the comments and finalize the extension for ratification by 10/22.

Please post any comments to this issue.

Thank you, Frank Galligan

[FrankGalligan]

From Alexey: The way how EdgeBreaker Connectivity section is laid out is optimized for encoders: "Split Data" section is located at the end because its contents isn't known until symbols are written. I'd argue that layout should be changed in favor of decoders (i.e. write "Split Data" first), because not every runtime environment has a notion of "file pointer" - e.g., on the web, random resource seek is not always an option so client will have to postpone decoding until "Split Data" arrival.

It reminds me the situation with "progressive MP4 streaming" ("moov" and "mdat" boxes): videos that start with "moov" could play almost instantly; otherwise client has to make several HTTP Range requests or download file entirely (not every server setup allows Range requests).

[FrankGalligan]

From Alexey: connectivity_method from "6.1 ParseSequentialConnectivityData()" seems to be unused in the spec. Yet, in the source code that parameter enables RANS indices compression.

[lexaknyazev]

• [x] Section 2.3 mentions push_back and assign from std::vector. But besides those, bitstream spec uses: back, begin, end, insert, and size. That section should mention them too.

• [x] The spec refers several times to pop while std::vector defines only pop_back. Looks like some containers must implement std::stack (or there're typos in the spec).

• [x] Also, what does .get() mean (e.g. in PredictionSchemeWrapDecodingTransform_ComputeOriginalValue)?

[lexaknyazev]

• [x] GetNumComponents() isn't defined.

• [x] ProcessInteriorEdges() isn't defined.

• [x] Statement with is_crease_edge_pos from MeshPredictionSchemeConstrainedMultiParallelogramDecoder_ComputeOriginalValues lacks function name.

• [x] ReadEncodedData isn't used, also title doesn't match:

• [x] .DecodeLeastSignificantBits32 from ParseEdgebreakerStandardSymbol isn't defined.

[FrankGalligan]

At this point I think we have addressed all open items. If anyone has any issues/comments/questions please post them here ASAP.

Thanks, Frank

[lexaknyazev]

• [x] Please replace [size] with actual names of variables that should be used in:

  • ParseMetadataElement
  • ParseSubMetadataKey
  • DecodeRawSymbols
  • DecodeTaggedSymbols

• [x] Consider naming constant integer values for variables like:

  • scheme in DecodeSymbols,
  • connectivity_method in DecodeSequentialConnectivityData, and probably other similar cases.

• [x] Float bitstream type doesn't specify its precision (assuming single, but this should be explicit).

• [x] f[] bitstream type isn't defined. It's used with variable length in DecodeTaggedSymbols and with length of 1 in ParseTopologySplitEvents. Is it an array of unaligned bits? How to interpret it?

• [ ] Since local vars lack types, it's not always obvious when division operator should be integer or float. Sure, this could be deduced from context, but I'd say that the spec should be explicit about that somehow.

• [x] In DecodeTaggedSymbols, max_symbol_bit_length_t has constant value 5, therefore ComputeRAnsUnclampedPrecision always returns 7 and ComputeRAnsPrecisionFromMaxSymbolBitLength always returns 12. So rans_precision_bits_t is always 12. Similar logic seems to be inlined in DecodeRawSymbols but with variable max_bit_length. Why are those two helper functions needed?

[lexaknyazev]

• [x] Extra dot:

[lexaknyazev]

• [x] What does .data() mean in DecodeAttributeSeams, DecodePredictionData_ConstrainedMulti, DecodePredictionData_TexCoords, DecodePredictionData_GeometricNormal, and MeshPredictionSchemeConstrainedMultiParallelogramDecoder_ComputeOriginalValues?

[lexaknyazev]

• [x] Consider harmonizing array semantics in bitstream types: UI8[size] vs size * UI8.

[lexaknyazev]

• [x] EdgebreakerValenceDecodeSymbol and ParseEdgebreakerStandardSymbol don't have arguments, but they're used with sym in EdgebreakerDecodeSymbol.

[lexaknyazev]

• [x] EdgebreakerValenceDecodeSymbol refers to undefined edge_breaker_symbol_to_topology_id array.

[lexaknyazev]

• [x] CornerToVert accepts two arguments (att_dec, corner_id), but all its usages in NewActiveCornerReached provide only one.

[lexaknyazev]

• [x] After https://github.com/google/draco/commit/308b812c2caf14a31cedf66dea3a468ee3469d61, curr_att_dec is being used in NewActiveCornerReached before its first initialization to 0 in DecodeAttributeData. Should connectivity decoder always use 0? If so it could be a separate constant value to keep curr_att_dec in scope of attribute data decoding.

[lexaknyazev]

Small question regarding f[n]:

When bit reading is finished it will always pad the read to the current byte.

• [x] When f[n] is being read in a loop, should this padding happen after loop ends or after each value?

[lexaknyazev]

• [x] In DecodeTaggedSymbols before https://github.com/google/draco/pull/251/commits/cf02c772fe87992c45f5688ce82659a7918fa39b, rans_precision was equal to 4096 and l_rans_base to 16384. After that commit, rans_precision became undefined and one of two occurrences of l_rans_base has been changed to a different constant value (4096).

[lexaknyazev]

• [ ] PosOpposite and SetPosOpposite refer to undefined opposite_corners_ array. What length should it have? Draco source suggests that opposite_corners_ consists of int32 elements and its length is num_faces * 3.

[lexaknyazev]

• [x] NewActiveCornerReached refers to undefined split_active_corners.

[lexaknyazev]

• [x] NewActiveCornerReached sets unused topology_edge_list values.

[lexaknyazev]

• [x] What buffer should ParseEdgebreakerStandardSymbol use? Is bit_symbol_buffer somehow related to eb_symbol_buffer?

[pjcozzi]

Wow, thanks for all the feedback here!!! @FrankGalligan please close this when ready.

[lexaknyazev]

• [ ] MapCornerToVertex assigns to uninitialized corner_to_vertex_map_[0].

[lexaknyazev]

• [x] Section 2.3 should mention .empty() from std::vector. It's used by EdgeBreakerTraverser_ProcessCorner, EdgeBreakerAttributeTraverser_ProcessCorner, and PopNextCornerToTraverse.

[lexaknyazev]

• [x] first_vert_id is used as function in RecomputeVerticesInternal.

[lexaknyazev]

• [x] Is static_cast<bool> in ConvertSymbolToSignedInt needed to be in the spec?

[lexaknyazev]

• [x] DecodeTaggedSymbols still refers to uninitialized l_rans_base.

[lexaknyazev]

• [x] IsTopologySplit uses pointer syntax (*) for writing out out_face_edge and out_encoder_split_symbol_id; MeshPredictionSchemeTexCoordsPortablePredictor_ComputePredictedValue uses pointer syntax for writing out predicted_value_. Other functions don't use *. Is it intended?

[zellski]

Wow. I can't decide if I should fear or long for the day when @lexaknyazev does code review on anything I wrote. 🥇

[FrankGalligan]

The spec should not be using pointers. Removed.

[lexaknyazev]

• [x] last_vert_added in NewActiveCornerReached isn't initialized.

[lexaknyazev]

• [x] There're slightly different usage semantics of ans_decoder_:

  • ProcessInteriorEdges and DecodeAttributeSeams have local declarations of ans_decoder_ of undefined type AnsDecoder. There're no such declarations in other relevant places.

  • DecodeTaggedSymbols and DecodeRawSymbols use different variable name: ans_.

  • RabsDescRead calls in DecodeAttributeSeams, DecodePredictionData_ConstrainedMulti, DecodePredictionData_TexCoords, and DecodePredictionData_GeometricNormal use & prefix for ans_decoder_. There's no & in other ans_decoder_ usages.