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

Write out CHAP frames in start_time order, ref #506 #539

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

Conversation

ping
Copy link

@ping ping commented Sep 25, 2021

This is my attempt at a fix for #506 to write out CHAP frames in start_time order (due to issues with ffmpeg converting mp3 to m4b).

Tried to keep the changes to a minimal but it may look a little hacky.

@ikalnytskyi
Copy link

Alternatively, we can maintain the sorting logic inside the sort_key function.

        def sort_key(items):
            frame, data = items
            frame_key = frame.HashKey
            frame_size = len(data)

            # Let's ensure chapters are always sorted by their 'start_time'
            # and not by size/element_id pair.
            if frame.FrameID == "CHAP":
                frame_key = frame.FrameID
                frame_size = frame.start_time

            return (get_prio(frame), frame_key, frame_size)

@ping
Copy link
Author

ping commented Apr 16, 2023

Hi, is there anything I can change to get this PR moving?

I'll be happy to incorporate the changes suggested (sort in sort_key).

@phw
Copy link
Collaborator

phw commented Apr 18, 2023

@ping Thanks for getting back to this. Yes, I think implementing this sorting as part of sort_key would be the preferred solution. If that is done the change could be merged.

@ping
Copy link
Author

ping commented Apr 18, 2023

@phw
I think there is a typo with the original suggestion. The return order should be priority, size, key.

return (get_prio(frame), frame_key, frame_size)

should be

return (get_prio(frame), frame_size, frame_key)

The PR has been updated with the changes.

@ping
Copy link
Author

ping commented May 29, 2023

@phw @lazka

I giving this a bump to see if this can be merged before the next release. The only failed test in CI seems to be due to timeout.

amake added a commit to amake/video2pod that referenced this pull request Oct 9, 2023
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.

3 participants