Open FrankGalligan opened 6 years ago
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).
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.
[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
)?
[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.
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
[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?
.data()
mean in DecodeAttributeSeams
, DecodePredictionData_ConstrainedMulti
, DecodePredictionData_TexCoords
, DecodePredictionData_GeometricNormal
, and MeshPredictionSchemeConstrainedMultiParallelogramDecoder_ComputeOriginalValues
?UI8[size]
vs size * UI8
.EdgebreakerValenceDecodeSymbol
and ParseEdgebreakerStandardSymbol
don't have arguments, but they're used with sym
in EdgebreakerDecodeSymbol
.EdgebreakerValenceDecodeSymbol
refers to undefined edge_breaker_symbol_to_topology_id
array.CornerToVert
accepts two arguments (att_dec, corner_id)
, but all its usages in NewActiveCornerReached
provide only one.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.Small question regarding f[n]
:
When bit reading is finished it will always pad the read to the current byte.
f[n]
is being read in a loop, should this padding happen after loop ends or after each value?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
).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
.NewActiveCornerReached
refers to undefined split_active_corners
.NewActiveCornerReached
sets unused topology_edge_list
values.ParseEdgebreakerStandardSymbol
use? Is bit_symbol_buffer
somehow related to eb_symbol_buffer
?Wow, thanks for all the feedback here!!! @FrankGalligan please close this when ready.
MapCornerToVertex
assigns to uninitialized corner_to_vertex_map_[0]
..empty()
from std::vector
. It's used by EdgeBreakerTraverser_ProcessCorner
, EdgeBreakerAttributeTraverser_ProcessCorner
, and PopNextCornerToTraverse
.first_vert_id
is used as function in RecomputeVerticesInternal
.static_cast<bool>
in ConvertSymbolToSignedInt
needed to be in the spec?DecodeTaggedSymbols
still refers to uninitialized l_rans_base
.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?Wow. I can't decide if I should fear or long for the day when @lexaknyazev does code review on anything I wrote. 🥇
The spec should not be using pointers. Removed.
last_vert_added
in NewActiveCornerReached
isn't initialized.[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.
ParseEdgebreakerConnectivityData
reads value of num_new_vertices
but it is never used because that variable is set to 0
at the beginning of RecomputeVerticesInternal
.num_corners
from Attribute_AssignPointsToCorners
isn't used.DecodePredictionData_ConstrainedMulti
, DecodePredictionData_TexCoords
, and DecodePredictionData_GeometricNormal
refer to undefined data
and pos
.orientaitons
and pred_tex_coords_orientaitons
.SubtractInt64Vectors
is not defined. Should it have the same behavior as SubtractVectors
?pred_vals
isn't defined for MeshPredictionSchemeParallelogramDecoder_ComputeOriginalValues
.att_dec_data_id
from ParseAttributeDecodersData
isn't used.att_dec_att_type
, att_dec_data_type
, att_dec_normalized
, and att_dec_unique_id
from ParseAttributeDecodersData
aren't used for decoding. Are these vars intended for application usage? If so, spec should define enums for att_dec_*_type
.point_to_corner_map
is used. Could it be replaced with a simple counter?att_connectivity_seam_opp
are never used.Hello, Great work on this extension and detailed format specification ! I know the comment deadline is long past, however while implementing an "inspector" to understand what the compression was doing, I came across 2 issues in the current specification :
DecodeSymbols
in EdgeBreakerTraversalValenceStart
should be 1, not 0 (which could lead to an infinite loop)TOPOLOGY_L
case in NewActiveCornerReached
is incorrect (generated triangle vertices are permuted). See here for my suggestion to fix this in the specificationI verified that both changes do match the c++ code.
It is possible to reproduce a test case for the second issue using the just-released online gltf tools:
Tools
, then Draco Compressor
trace
in the draco compressor sub-menu, then reclick on Draco Compressor
(as the mesh is already compressed, it goes to an inspection mode, using the inspector linked above to extract info about the encoding)currentSpec
in the draco compressor sub-menu, then reclick on Draco Compressor
I hope this helps :)
I know the comment deadline is long past...
@JeremieA thanks for the careful review. It is never too late to make "bug fixes" to the spec and bigger changes can always go into a future version. @FrankGalligan and company will know best where things should land.
@FrankGalligan should I leave this open until all the checkboxes are checked? Do you want to move any tasks to new issues in the Draco GitHub repo?
Bump @FrankGalligan
should I leave this open until all the checkboxes are checked? Do you want to move any tasks to new issues in the Draco GitHub repo?
@FrankGalligan
AttrOpposite
declares two arguments but uses only one:
@FrankGalligan
att_dec_data_id
type mismatch with implementation
@FrankGalligan
face_id
assigned in DecodeAttributeSeams
is never used.@FrankGalligan
local
assigned in DecodeAttributeSeams
is never used.@FrankGalligan
[ ] att_dec_decoder_type
array is zero-based:
however its usage may imply otherwise. E.g. in
IsOnBoundary
:
Here, the value of att_dec_decoder_type[0]
will never be read. Moreover, IsOnBoundary
will never be called with att_dec == 0
because it's called only from EdgeBreakerAttributeTraverser_ProcessCorner
:
which can be called only when curr_att_dec
is not 0
:
Similar concerns could be raised about Opposite
:
@FrankGalligan any update on the above questions from @lexaknyazev?
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