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
310 stars 56 forks source link

buffer.readDoubleBE is not a function at readInt64BEasFloat #205

Closed HDv2b closed 10 months ago

HDv2b commented 11 months ago

I'm trying to get data from a GoPro 10 video in browser (latest Chrome).

I'm using gpmfExtractWrapper and passing the data directly to :

            goProTelemetry(rawData, {
              stream: ['GPS'],
              progress: console.log,
            }).then(console.log)

and immediately get the following dump:

Uncaught (in promise) TypeError: buffer.readDoubleBE is not a function
    at readInt64BEasFloat (webpack-internal:///(:4200/app-pages-browser)/../../node_modules/gopro-telemetry/code/utils/findFirstTimes.js:8:51)
    at module.exports (webpack-internal:///(:4200/app-pages-browser)/../../node_modules/gopro-telemetry/code/utils/findFirstTimes.js:61:14)
    at eval (webpack-internal:///(:4200/app-pages-browser)/../../node_modules/gopro-telemetry/index.js:145:37)
    at Array.map (<anonymous>)
    at process (webpack-internal:///(:4200/app-pages-browser)/../../node_modules/gopro-telemetry/index.js:145:28)
    at async GoProTelemetry (webpack-internal:///(:4200/app-pages-browser)/../../node_modules/gopro-telemetry/index.js:347:18)
readInt64BEasFloat @ findFirstTimes.js:7
module.exports @ findFirstTimes.js:60
eval @ index.js:145
process @ index.js:145
setTimeout (async)
eval @ breathe.js:7
module.exports @ breathe.js:7
process @ index.js:115
GoProTelemetry @ index.js:347
eval @ VideoUploadContext.tsx:71
Promise.then (async)
reader.onload @ VideoUploadContext.tsx:69
load (async)
addVideo @ VideoUploadContext.tsx:61
handleDrop @ DropWrapper.tsx:103
callCallback @ react-dom.development.js:19467
invokeGuardedCallbackImpl @ react-dom.development.js:19516
invokeGuardedCallback @ react-dom.development.js:19591
invokeGuardedCallbackAndCatchFirstError @ react-dom.development.js:19605
executeDispatch @ react-dom.development.js:30661
processDispatchQueueItemsInOrder @ react-dom.development.js:30693
processDispatchQueue @ react-dom.development.js:30706
dispatchEventsForPlugins @ react-dom.development.js:30717
eval @ react-dom.development.js:30907
batchedUpdates$1 @ react-dom.development.js:23771
batchedUpdates @ react-dom.development.js:27623
dispatchEventForPluginEventSystem @ react-dom.development.js:30906
dispatchEvent @ react-dom.development.js:28679
dispatchDiscreteEvent @ react-dom.development.js:28650

When I breakpoint the function exported in findFirstTimes.js, I see that the object passed in to the data param is a Uint8Array with 1645084 bytes and forceGPSSrc is null

Hope that helps, thanks

JuanIrache commented 11 months ago

I didn't work on the conversion for browsers, so I'll wait for others to chime in

HDv2b commented 10 months ago

@Akxe Is this something you have some idea about?

Akxe commented 10 months ago

What version are you using, 1.2.1 or 1.2.2? The error is probably something about the fact that the browser does not support Buffer on its own but thought a polyfill.

If you want to use the lib in the browser, I suggest using 1.2.1, I made it to be compatible with browsers and I have been using it a lot in the last year (about 50,000, files parsed).

Akxe commented 10 months ago

@JuanIrache What was the reason for the revert? I might try to find some time to fix it... If you could set up tests for the broken use case I would love to fix it for you :)

JuanIrache commented 10 months ago

The browser implementation broke features that were working previously, at least reusing parsedData. The library was not initially designed for browser use. It's a nice extra, but we should find a way to keep preexisting features. The GitHub master branch currently brings back the broken features, and this seems to have broken browser compatibility. This has not been published to NPM yet (so installing via NPM should allow browser usage), but eventually, it will be.

Message ID: @.***>

Akxe commented 10 months ago

If I read it correctly, you are having a "broken" Buffer polyfill that does not support all methods it should. Try to remove the Buffer polyfill and it should start working

HDv2b commented 10 months ago

Unfortunately it looks like Buffer is bundled and polyfilled as part of next.js.

Would it work to do, for example:


if (
  typeof Buffer !== 'undefined' 
  && typeof  buffer.readUInt8  !== 'undefined' 
  && typeof buffer.readUInt16B  !== 'undefined' 
  && typeof buffer.readInt32BE !== 'undefined' 
  && typeof buffer.readDoubleBE !== 'undefined' 
) {
  readUInt8 = (buffer) => buffer.readUInt8(0);
  readUInt16BE = (buffer) => buffer.readUInt16BE(0);
  readInt32BE = (buffer) => buffer.readInt32BE(0);
  readInt64BEasFloat = (buffer, offset) => buffer.readDoubleBE(offset);
} else {
...

to ensure all functions are available?
Akxe commented 10 months ago

There are two things. NextJS should bundle proper Buffer polyfill. And this fix is also cool... Who knows, maybe the if should be reversed. Use DataView if available and buffer as the fallback.

Your code is invalid as buffer is something else to Buffer. Corrected:

if (
  typeof Buffer !== 'undefined'
  && ['readUInt8', 'readUInt16BE', 'readInt32BE', 'readDoubleBE'].every(fn => Buffer.prototype[fn])
) { ... }
HDv2b commented 10 months ago

NextJS should bundle proper Buffer polyfill

Agreed, unfortunately it seems the package it uses it the de-facto standard in the js community

I did try exactly the code you suggested and it didn't quite work (at first it was promising as it ran past a few breakpoints, but then the same error just came up again further down the processing) so I tried the other suggestion of trying DataView first and it worked perfectly

that said I'm not sure what to do by way of PR:

https://github.com/JuanIrache/gopro-telemetry/compare/master...HDv2b:gopro-telemetry:fix-browser?expand=1

Akxe commented 10 months ago

@JuanIrache Would you do the honors? I am not sure how to do the tagging of the branch and so on...

if (DataView) {
  readUInt8 = (buffer) => new DataView(buffer.buffer).getUint8(0);
  readUInt16BE = (buffer) => new DataView(buffer.buffer).getUint16(0);
  readInt32BE = (buffer) => new DataView(buffer.buffer).getInt32(0);
  readInt64BEasFloat = (buffer, offset) => new DataView(buffer.buffer).getFloat64(offset);
} else if (typeof Buffer !== 'undefined' ) {
  readUInt8 = (buffer) => buffer.readUInt8(0);
  readUInt16BE = (buffer) => buffer.readUInt16BE(0);
  readInt32BE = (buffer) => buffer.readInt32BE(0);
  readInt64BEasFloat = (buffer, offset) => buffer.readDoubleBE(offset);
} else {
  throw new Error('Please install a compatible `Buffer` or `DataView` polyfill');
}

To explain why. The first (if (DataView)) is a fix to the bug mentioned here (not fully spec-compliant Buffer implementation). The second if to allow webpack, rollup and similar bundlers to treat the other branch of code as optional and thus marking the Buffer requirement as optional. Lastly we fallback to an error to provide user with clear instruction on what to do next.

JuanIrache commented 10 months ago

Thanks. I made the change to the browser branch, which comes from 1.2.1. I haven't merged this into Master for now, because it breaks the parseData workflow (#201). The test that should check for that is not actually failing when it should. I need to come up with a new one, but struggling to find the time.

Also, not sure why but testing times in CircleCI have skyrocketed, I have been increasing timeouts across the board to try and pass the test.

HDv2b commented 10 months ago

Thanks guys, I recommend you still keep the code you suggested also

if (
  typeof Buffer !== 'undefined'
  && ['readUInt8', 'readUInt16BE', 'readInt32BE', 'readDoubleBE'].every(fn => Buffer.prototype[fn])
) { ... }

since without it there's no way of realising you have an incompatible polyfill