marek-simonik / record3d-wifi-streaming-and-rgbd-mp4-3d-video-demo

MIT License
29 stars 7 forks source link

fix json metadata parsing for our specific examples #1

Open jaeh opened 3 years ago

jaeh commented 3 years ago

hi marek.

we installed the record3d app, thank you for your work, it's awesome and works very nicely on our hardware, very much worth the money :)

when trying to preview the videos using the code in this repository, we got a json parsing error though:

Uncaught SyntaxError: JSON.parse: unexpected non-whitespace character after JSON data at line 1 column 107 of the JSON data

something goes wrong with the line of parsing code i changed, seems there is no newline after the json (anymore?) making the meta string include binary data in the json parse step.

i am not sure if my "fix" will work for all examples, it does for all videos we tested it with, regardless if they were taken with the lidar functionality, or taken without, and if i understand the video data + metadata of the intrinsicMatrix correctly it should work in all cases.

i also have some questions and feature suggestions for record3d, which repository do you want me to file those as issues in?

marek-simonik commented 3 years ago

Hi Jascha,

thank you for spotting this! I have just tried to replay an exported RGBD Video via http://record3d.xyz and I was unable to reproduce the bug you experienced.

May I ask which Record3D version and iOS/iPadOS version are you using? I have tried exporting into an RGBD mp4 video on both iOS and iPadOS 14.5.1 without any problems related to this demo. My guess is that this change might be due to a different iOS version.

My ugly workaround for parsing the metadata out of the mp4 file is definitely not clean in any way, so I will have to find a better way to do this in the future.

In the meantime (while continuing with my ugly parsing workaround), wouldn't it be better to search for the rightmost } instead of the first ]}? Although your fix works for current videos, I might extend the metadata JSON in the future and ]} could pop up in more places (whereas the JSON should always end with a trailing }).

I am glad you created this pull request so that other people with similar problem can see this!

jaeh commented 3 years ago

hi again :)

ios version: 14.4.2 ipad pro (11 inch, 2nd generation)

the record 3d app should be up to date, installed a few days ago, i could not find a version number for it, adding the version number in settings -> about for future references might be a good addition for the next release :) (not an ios user, and have no idea how to check the version of an installed app, but we installed a few days ago so we should have 1.5.6 installed)

we are updating ios now and i will amend this comment once we know if the newer version works.

if i am thinking correctly, the problem with finding the last } would be that js would parse the full meta string twice, which includes all of the video data after the intrinsicMatrix json. see the code block at the end of this comment for a way to avoid parsing the video data at all.

1. loop, does not work

my first approach using

function findFirstNonAsciiIndex(str) {
  for (let i = 0; i < str.length; i++) {
    if (str.charCodeAt(i) > 255) {
      return i
    }
  }

  return str.length - 1
}

does not find a match and loops over the full binary data, freezing the browser in the process, so that seems to not work. (according to https://www.ascii-code.com/ 255 is the last valid ascii character code)

2. breaking change and extra data in json

maybe adding a breaking change, falling back to ]} if the condition does not trigger could work:

{ "intrinsicMatrix": [], "eof": 1} 

then using string.search would be more robust, performant and take less memory, albeit the solution being inelegant too and using regex, not sure if i like that too much.

the "eof" part could be added once you want to add other data to the metadata, with the fallback triggering all the time until that change lands in record3d, making all old videos work forever.

let meta1 = '{"intrinsicMatrix": [],"eof":1}'
let meta2 = '{"intrinsicMatrix": [], "eof": 1}'

const index1 = meta1.search(/,\s?"eof/)
const index2 = meta2.search(/,\s?"eof/)

const result1 = meta1.substr(0, index1) + '}'
const result2 = meta2.substr(0, index2) + '}'

console.log(result1 === result2)

3. performance nitpick

another small nitpick, would be relevant for huge videos, but compared to the rest of the script this will still only be a minor speedup:

console.time('lastindexof')
let meta = fileContents.substr(fileContents.lastIndexOf('{"intrinsic'));
console.timeEnd('lastindexof')

console.time('indexof')
let meta2 = fileContents.substr(fileContents.indexOf('{"intrinsic'));
console.timeEnd('indexof')

console.log(meta === meta2) // is true, we just have to assume that intrinsicMatrix only appears once

// lastindexof: 53ms
// indexof: 1ms 

once we have a plan how to move forward i can append the pull request with the changes you decided upon :)

marek-simonik commented 3 years ago

Thank you for the thorough overview. The app version should be visible on the Record3D's App Store page, but I can confirm that 1.5.6 is currently indeed the latest version. I am going to display the version number directly inside the app (in the Settings tab) beginning with the next update.

I assume that updating to the latest iOS version did not fix the issue for you since you did not update your comment. If possible, would you consider sending me sample .mp4 and .r3d files at support@record3d.app please? I would ideally want to figure out what is causing this bug instead of just fixing the consequences. Thank you :)!

jaeh commented 3 years ago

hi :) we just got back from our excursion today and i got to test another bunch of videos, so far it seems that after the update ios writes the newline into the video correctly, i still get an error with the old video on record3d.xyz, the new videos load flawlessly.

marek-simonik commented 3 years ago

Hi, it seems like the bug is indeed related to a change in iOS. Can you please try to re-export an old 3D Video (which you shot on iOS 14.4.2) into .mp4 on the latest version of iOS and test if the new .mp4 is playable? (It is needed to delete the already exported old .mp4 to be able to export into .mp4 again.)

Could you please also share either an example .mp4 video that shows this bug or just the part of the mp4 file that begins with {"intrinsic till the end of the file? I would like to test different approaches to parsing the JSON without bothering you with my requests to try try them out :). Thank you!