Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Las1.1 support is failing, forcing the header to 1.2 seems to work? #61

Closed
wants to merge 3 commits into from
Closed

Las1.1 support is failing, forcing the header to 1.2 seems to work? #61

wants to merge 3 commits into from

Conversation

TerjeWiigMathisen
Copy link

This should not be merged!
It is just a proof of concept that when reading a las 1.1 file fails, forcing the version field to 1.2 does work. I have spent a couple of days trying to trace into the libraries to discover where the panic on 1.1 is happening but have not been able to do so, sorry!

@gadomski gadomski marked this pull request as draft August 25, 2023 12:15
@gadomski
Copy link
Owner

Thanks, I've converted to draft and will take a look when I get a chance. Appreciate it!

@gadomski
Copy link
Owner

One thing to check is that the point format that you're reading is supported by las 1.1, which only allows point formats 0 and 1. One quick way to check is with PDAL:

$ pdal info tests/data/autzen.las --metadata | jq .metadata.dataformat_id  
1

If you can provide an example file that fails, that could also help debugging.

@TerjeWiigMathisen
Copy link
Author

TerjeWiigMathisen commented Aug 25, 2023 via email

@TerjeWiigMathisen
Copy link
Author

TerjeWiigMathisen commented Aug 25, 2023 via email

@gadomski
Copy link
Owner

The file was so tiny that I'm just including it!

Hm, I don't see it attached anywhere, might have to provide a link or send some other way.

@TerjeWiigMathisen
Copy link
Author

TerjeWiigMathisen commented Aug 26, 2023 via email

This was referenced Aug 28, 2023
Closed
Closed
@gadomski
Copy link
Owner

Thanks for providing the file! I think it got stripped by Github (I don't think you can attach files via email to Github comments).

It appears as though the file you provided has a GPS Time Type field set to standard, which is not supported by LAS 1.1:

$ cargo run --features laz --example count ~/Downloads/32-1-472-150-76.laz 
   Compiling las v0.8.1 (/Users/gadomski/Code/gadomski/las-rs)
    Finished dev [unoptimized + debuginfo] target(s) in 1.78s
     Running `target/debug/examples/count /Users/gadomski/Downloads/32-1-472-150-76.laz`
thread 'main' panicked at 'Unable to open reader: Feature { version: Version { major: 1, minor: 1 }, feature: "GpsStandardTime" }', examples/count.rs:12:46
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

Here's the relevant info from the las 1.2 standard:

GPS Absolute Time (as well as GPS Week Time) – LAS 1.0 and LAS 1.1 specified GPS
“Week Time” only. This meant that GPS time stamps “rolled over” at midnight on
Saturday. This makes processing of LIDAR flight lines that span the time reset difficult.
LAS 1.2 allows both GPS Week Time and Absolute GPS Time (POSIX) stamps to be used.

So las-rs is behaving correctly, if overly strictly and with poor error messages. I've opened two issues to track those problems:

Note that las-rs is unsupported+unfunded, so I can't really give a timeline when these things will be fixed -- it'll have to happen on my own time. In the meantime, your workaround of upgrading the version seems reasonable.

As an aside, you mentioned that you're using lastools -- I would recommend checking out PDAL, as it is actively maintained and comes with a lot of batteries included. It reads permissively and writes strictly, so you could use it to "fix" your files, e.g.:

pdal translate 32-1-472-150-76.laz 32-1-472-150-76.fixed.laz

Hope that helps!

@TerjeWiigMathisen
Copy link
Author

TerjeWiigMathisen commented Aug 28, 2023 via email

@gadomski
Copy link
Owner

No other LiDAR software I have seen cares about what happens to lie after
the first null!

Yup, that's probably true ... I wrote las-rs to be strict to the spec, and I didn't really have a real-world user in mind :-). But since folks are using it, it would make sense to update the library to read more permissively.

Do you want a copy of this one as well?

Yes please!

@TerjeWiigMathisen
Copy link
Author

TerjeWiigMathisen commented Aug 28, 2023 via email

@TerjeWiigMathisen
Copy link
Author

TerjeWiigMathisen commented Aug 28, 2023 via email

@gadomski
Copy link
Owner

The mapping from each version to the features it supports is here:

las-rs/src/feature.rs

Lines 75 to 88 in bea51e5

features! {
/// Does the header allow a file source id, or is that field reserved?
FileSourceId(1, 2, 3, 4);
/// Is there a bit flag to set the type of time value in each point?
GpsStandardTime(2, 3, 4);
/// Does this file support waveforms?
Waveforms(3, 4);
/// Is there a bit flag to indicate synthetic return numbers?
SyntheticReturnNumbers(3, 4);
/// Does this file support 64-bit point counts?
LargeFiles(4);
/// Does this file support extended variable length records?
Evlrs(4);
}

The checks happen in Builder::into_header:

las-rs/src/header/builder.rs

Lines 176 to 198 in bea51e5

if self.version.requires_point_data_start_signature()
&& (n < 2 || self.vlr_padding[n - 2..] != POINT_DATA_START_SIGNATURE)
{
self.vlr_padding.extend(&POINT_DATA_START_SIGNATURE);
}
if self.file_source_id != 0 {
self.version.verify_support_for::<FileSourceId>()?;
}
if self.has_synthetic_return_numbers {
self.version
.verify_support_for::<SyntheticReturnNumbers>()?;
}
if self.gps_time_type.is_standard() {
self.version.verify_support_for::<GpsStandardTime>()?;
}
// TODO check waveforms
if !self.version.supports_point_format(self.point_format) {
return Err(Error::Format {
version: self.version,
format: self.point_format,
}
.into());
}

If we were going to make things permissive, we might have a boolean attribute on Builder called upgrade_version_to_support_header or something (that's a terrible name, I haven't thought much about this). Then, we would replace those self.version.verify_support_for calls with a new self.verify_support_for_or_upgrade, which would check the upgrade boolean and either return an Err or upgrade the version, w/ a warning. There might be some strange edge cases to handle, but that's how I picture things going more-or-less.

@TerjeWiigMathisen
Copy link
Author

TerjeWiigMathisen commented Aug 28, 2023 via email

@gadomski gadomski mentioned this pull request May 30, 2024
Merged
@gadomski gadomski closed this in #83 May 30, 2024
@gadomski
Copy link
Owner

@TerjeWiigMathisen I attempted to reproduce the "bytes after null" error for https://tmsw.no/o/33-1-501-284-40.laz, and couldn't:

$ cargo run -F laz --example count ~/Downloads/33-1-501-284-40.laz 
Number of points: 962652

Can you provide a reproducible example of the error? Thanks!

@TerjeWiigMathisen
Copy link
Author

TerjeWiigMathisen commented May 31, 2024 via email

@TerjeWiigMathisen
Copy link
Author

TerjeWiigMathisen commented May 31, 2024 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@TerjeWiigMathisen @gadomski