-
Notifications
You must be signed in to change notification settings - Fork 58
Add custom_frame_mappings tutorial and optimizations #887
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
base: main
Are you sure you want to change the base?
Conversation
@@ -501,7 +511,7 @@ void SingleStreamDecoder::addVideoStream( | |||
customFrameMappings.has_value(), | |||
"Missing frame mappings when custom_frame_mappings seek mode is set."); | |||
readCustomFrameMappingsUpdateMetadataAndIndex( | |||
streamIndex, customFrameMappings.value()); | |||
activeStreamIndex_, customFrameMappings.value()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This resolves a bug that raised Runtime Error: bad_optional_access
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's surprising, streamIndex
isn't an optional. Do you remember what was causing the issue? Should we add a test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect the issue was not with streamIndex
variable itself, but accessing an unset value in StreamInfo[streamIndex]
on videos with multiple streams. I can spend some time digging into this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @Dan-Flores , the benchmark results make sense, I left a few comments below.
In terms of narration, the structure of the tutorial makes sense as well, it closely follows the seek_mode
one which is a good thing. I think the main conclusion would apply: passing custom frame mappings speeds up the VideoDecoder
instanciation (not the frame decoding itself!). We should try to make that clear throughout the tutorial.
It may also be interesting to draw bridges between this tutorial and the seek_mode one. In the seek_mode tutorial we contrast the use of approximate with exact, basically saying approximate is faster while being slightly less accurate. We should make the point that passing frame mappings gives you the best of both worlds: fast instanciation and accurate seeking.
@@ -501,7 +511,7 @@ void SingleStreamDecoder::addVideoStream( | |||
customFrameMappings.has_value(), | |||
"Missing frame mappings when custom_frame_mappings seek mode is set."); | |||
readCustomFrameMappingsUpdateMetadataAndIndex( | |||
streamIndex, customFrameMappings.value()); | |||
activeStreamIndex_, customFrameMappings.value()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's surprising, streamIndex
isn't an optional. Do you remember what was causing the issue? Should we add a test?
print(f"Running benchmarks on {Path(video_path).name}") | ||
print("Creating a VideoDecoder object with custom_frame_mappings JSON str from file:") | ||
with open(json_path, "r") as f: | ||
bench(VideoDecoder, source=video_path, stream_index=stream_index, custom_frame_mappings=(f.read())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can simplify the benchmark a bit and only benchmark passing f
instead of f.read()
Ultimately the main difference between these 2 will be the speed of the file system, I don't feel like it's super relevant in this specific tutorial which should be a fairly straightforward entry point
1e24ed6
to
dbd6610
Compare
This PR adds the code that will become the
custom_frame_mappings
tutorial.Currently, 3 benchmarks are run using a short video (30 seconds) and a long video (10 minutes) :
Additionally, some fixes / optimizations were added to
custom_frame_mappings
logic inSingleStreamDecoder.cpp
to improve the performance.Benchmark results:
Compare performance of initializing VideoDecoder with custom_frame_mappings vs seek_modes
Running benchmarks on short_video.mp4
Creating a VideoDecoder object with custom_frame_mappings:
med = 127.90ms +- 14.34
Creating a VideoDecoder object with seek_mode='exact':
med = 173.68ms +- 8.05
Running benchmarks on long_video.mp4
Creating a VideoDecoder object with custom_frame_mappings:
med = 53.55ms +- 2.04
Creating a VideoDecoder object with seek_mode='exact':
med = 215.63ms +- 15.46
Decode frames with custom_frame_mappings vs exact seek_mode
Running benchmarks on short_video.mp4
Decoding frames with custom_frame_mappings JSON str from file:
med = 316.99ms +- 10.47
Decoding frames with seek_mode='exact':
med = 358.02ms +- 9.26
Running benchmarks on long_video.mp4
Decoding frames with custom_frame_mappings JSON str from file:
med = 245.47ms +- 6.73
Decoding frames with seek_mode='exact':
med = 419.14ms +- 20.70