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.

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

Initial refactor #1

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Initial refactor #1

wants to merge 5 commits into from

Conversation

ryanhausen
Copy link
Collaborator

Only refactors backend code. UI needs to be refactored too.

Able to convert and save a single file. Example seen in demo.py

I would like to refactor a bit more in the final product. I want to put everything in a package along with the UI code.

@ryanhausen ryanhausen requested a review from glemson August 31, 2023 14:43
Copy link

@glemson glemson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some comments and requests for a few changes.

@@ -0,0 +1,2 @@
[settings]
profile=black
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what does this do?

3. Extract the GPS data from the video.
4. Load the video, audio, and GPS data into the repository.

Args:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not consistent with the actual arguments of the function


def process_video(
video_path: str,
audio_path: str,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this the desired output path?

import job_config as jc


# TODO: Maybe this should stream to file instead of loading everything into memory
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

slight disadvantage is that the separate loading would not be part of the same transaction.

if "Altitude (ft)" in df.columns:
df["Altitude (m)"] = df["Altitude (ft)"] * 0.3048
else:
df["Altitude (m)"] = -1.0
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in the Netherlands for example altitude -1 is not uncommon.
can we make somehow ensure that missing data would be inserted as a DB NULL?
But NULL would also be more explicitly indicating missing data than some number.

DROP TABLE dbo.video;
END

CREATE TABLE dbo.video (
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be good to add a "video_create_date datetime" column. this data is, I think, contained in the JSON, but would be nice to have as explicit metadata. There may be some other fields we might also want to add as explicit columns, but this one is I think useful. for example to make absolute timestamps from relative times in text_segment.

# Extract the audio from the video
with utils.LogTime(logger, "Getting audio/metadata from video"):
vp.extract_audio(video_path, audio_path, logger=logger)
video_metadata = vp.get_metadata(video_path, logger=logger)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see DDL.sql: would be good I think to extract video_create_date from the metadata and make that an explicit field. so should be set here.
Btw, video_create_data iso just create_date as latter pattern is often used for when a row was added to the table.

gps_filename VARCHAR(1024) NOT NULL,
checksum VARCHAR(64) NOT NULL,
metadata VARCHAR(MAX) NOT NULL, -- JSON
CONSTRAINT PK_video PRIMARY KEY(id)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually would be good ALSO to have "last_update_date datetime default getdate()" here and all other tables so we see when rows were added.


# https://stackoverflow.com/a/39215961/2691018
class StreamToLogger:
"""This class redirects write calss to a logger."""
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

type: calss iso calls

assert os.path.exists(ffmpeg_binaries)
os.environ["PATH"] = f"{ffmpeg_binaries}:" + os.environ["PATH"]

base_path = "/home/idies/workspace/nist_ai/data/video/Tanya"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reminder that we just agreed we will put all videos under
/home/idies/workspace/nist_ai/data/video
and we'll have subfolders GoPro/, TrackAddict/, and Other/ without further hierearchy for now.

Copy link

@glemson glemson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

more comments

# - altitude_m: float
# - speed_kmh: float
with utils.LogTime(logger, "Parsing gps file"):
gps_data = gp.parse_gps_file(
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if I am not mistaken parse_gps_file throws an exception if the file does not exist which is not caught here. so does that mean there is no way to load videos without GPD file?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants