Skip to content

Added Frames and GOP Timecode Side Data Support - #60

Open
Hendo16 wants to merge 12 commits into
vansante:v2from
Hendo16:v2
Open

Added Frames and GOP Timecode Side Data Support#60
Hendo16 wants to merge 12 commits into
vansante:v2from
Hendo16:v2

Conversation

@Hendo16

@Hendo16 Hendo16 commented Jan 22, 2026

Copy link
Copy Markdown

Closed my previous PR as I hadn't removed the restricted frame output, as well as not fully building out the Frame struct.

Noticed that there wasn't any support to detect I-Frames with this library so quickly added arguments to the command that'll add it to the JSON output as well as building up the matching structs

@vansante

Copy link
Copy Markdown
Owner

So I had a little look, and codewise, everything seems in order. However, I am seeing a potential problem in the amount of data this extra option is going to generate:

-show_frames

Show information about each frame and subtitle contained in the input multimedia stream.

The information for each single frame is printed within a dedicated section with name "FRAME" or "SUBTITLE".

The amount of frames in some longer videos is going to be immense, and I would like this to remain a fast utility to grab information about media files. If we enable this by default, things will end badly on that end.

(For reference, the included test file test.mp4, which is just over 1 MiB, already clocks in at 381 frames 😅)

So I am fine with including the frame support JSON structs and all, but I don't think we should enable this option by default.

@Hendo16

Hendo16 commented Jan 26, 2026

Copy link
Copy Markdown
Author

Fair enough! Appreciate you checking this out - will update this so the option is removed from the default list of options

@vansante vansante left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Can you perhaps write a separate test with one of the test files in assets with the -show_frames option set and a test the structs are as expected?

(And please fix the CI thingy about capitalization)

Comment thread sidedata.go
MinBitrate int `json:"min_bitrate"`
AvgBitrate int `json:"avg_bitrate"`
BufferSize int `json:"buffer_size"`
VbvDelay FlexInt `json:"vbv_delay"`

@vansante vansante Feb 18, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Can you explain why/how this can be a string?
FFMpeg vbv_delay

And since this is the only place we use FlexInt, maybe we dont need it then?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

VbvDelay can be an integer or a string. I've encountered cases where it's either 0, -1 or "N/A" so parsing it just as an int doesn't work and needs to be handled accordingly. Maybe the better solution is just parsing the integers to a string as well?

@Hendo16

Hendo16 commented Feb 26, 2026

Copy link
Copy Markdown
Author

Sorry for the mess of commits - I fixed the capitalization issue and added some tests but for the life of me I can't figure out why the linter isn't working. It says there's a version mismatch so I tried updating the go.mod but then then that resulted in another error so i'm going to leave this for now.

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