JuanIrache / gopro-telemetry

Reads telemetry from the GPMF track in GoPro cameras (Hero5 and later) and converts it to multiple formats
https://tailorandwayne.com/gopro-telemetry-extractor/
MIT License
315 stars 57 forks source link

Either fix tests for @gmod/binary-parser or fix raw -> parsedData with binary-parser #201

Closed JuanIrache closed 1 year ago

JuanIrache commented 1 year ago

Changing to binary-parser here https://github.com/JuanIrache/gopro-telemetry/pull/193/commits/24cef0fc45c1b37212c7a342e1d287ecdf909195 breaks the raw option for some streams (GPS of new cameras). We need to either fix that or revert to @gmod/binary-parser (done in the dev branch) and fix the tests @Akxe

Akxe commented 1 year ago

I do not know what is broken; During development all tests passed.

JuanIrache commented 1 year ago

The raw to parsedData test passed with the hero6 sample, but it breaks with hero11. I modified the test in this branch: https://github.com/JuanIrache/gopro-telemetry/tree/debug_raw

Akxe commented 1 year ago

I have split the test to make it readable. The error is not with the parser but with the parsing.

When you call goproTelemetry({ parsedData, timing }), all timings will be changed a tiny bit.

      Array [
        Object {
          "cts": 0,
    -     "date": 2022-09-20T13:29:38.299Z,
    +     "date": 2022-09-20T13:29:36.898Z,
          "sticky": Object {
            "LRVO": 0,
            "LRVS": 1,
          },
          "value": 0,
        },
        Object {
          "cts": 38.5,
    -     "date": 2022-09-20T13:29:38.337Z,
    +     "date": 2022-09-20T13:29:36.936Z,
          "value": 0,
        },
        Object {
          "cts": 77,
    -     "date": 2022-09-20T13:29:38.375Z,
    +     "date": 2022-09-20T13:29:36.974Z,
          "value": 0,
        },
      ]

The same error is present for all 20 stream types of GoPro11.

Try my fork to see how it looks. I am not familiar with how parsing of already parsed files works...

https://github.com/Akxe/gopro-telemetry

JuanIrache commented 1 year ago

Thank you for looking into this.

I have copied your test into the (currently) dev branch (with the binary-parser reverted to gmod's) and it runs well (with some tweaks to account for missing streams): https://github.com/JuanIrache/gopro-telemetry/blob/dev/__tests__/node/index.test.js

In my real use case not only did the timing change, all the usable data was gone in the returned "raw" data. But I don't rule out that that's a consequence of the wrong timings, or at least caused by the same root issue. I haven't found the time to properly analyse this yet.

Akxe commented 1 year ago

I am not sure how to process this either. To me. It is really hard to test it, since I am not sure of the desired outcome 😕

Akxe commented 1 year ago

Do you have some more time to look into it? I am not sure what to do...

JuanIrache commented 1 year ago

I'm also not entirely sure what is breaking. I haven't been able to look into it yet, sorry.

JuanIrache commented 1 year ago

Ok. The problem was that with the changes for the browser, we are using BigInts in some places. Before, it was all Numbers. This means copying objects with JSON.parse(JSON.stringify()) was broken. I have implemented a fix in the browser branch. I don't know if this has an impact on performance. Will merge it in a few days if there are no concerns.

Akxe commented 1 year ago

The fix for bigInt vs Number is twofold. I can very easily switch the bigint to number, but I am not sure if we are guaranteed that the number never overflows the max int limit. Until now the function buffer.readDoubleBE(offset) would have thrown it the resulting number was too big, but given it did not happen until now, it probably won't...

This is the fix to convert bigint to number: Number(new DataView(buffer.buffer).getFloat64(offset)) will transform the only place that we are getting bigint back to number

JuanIrache commented 1 year ago

Yes. I don't think we actually need BigInts either, I have only seen them used in STMP. I will save a bigint branch in case we want to revert down the line.

JuanIrache commented 1 year ago

A note in case we come back to this in the future, most BigInts were generated by ParseV when the GoPro data had J values (uint64)