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

example: Add interval edit script #108

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

samdbmg
Copy link
Member

@samdbmg samdbmg commented Feb 3, 2025

Details

Adds a script that cuts between Flows on a fixed time interval, demonstrating sample_offset and sample_count.
Also updates the simple_edit script to set some custom tags on created Flows.

Jira Issue (if relevant)

Jira URL: https://jira.dev.bbc.co.uk/browse/CLOUDFIT-5392

Related PRs

Follows #107 (because you can't test this script without those fixes)

Submitter PR Checks

(tick as appropriate)

  • PR completes task/fixes bug
  • API version has been incremented if necessary
  • ADR status has been updated, and ADR implementation has been recorded
  • Documentation updated (README, etc.)
  • PR added to Jira Issue (if relevant)
  • Follow-up stories added to Jira

Reviewer PR Checks

(tick as appropriate)

  • PR completes task/fixes bug
  • Design makes sense, and fits with our current code base
  • Code is easy to follow
  • PR size is sensible
  • Commit history is sensible and tidy

Info on PRs

The checks above are guidelines. They don't all have to be ticked, but they should all have been considered.

@samdbmg samdbmg requested a review from a team as a code owner February 3, 2025 11:30
@samdbmg samdbmg mentioned this pull request Feb 3, 2025
11 tasks
Base automatically changed from sammg-fix-example-snags to main February 3, 2025 14:09
Adds a script that cuts between Flows on a fixed time interval,
demonstrating `sample_offset` and `sample_count`.
Also updates the `simple_edit` script to set some custom tags on created
Flows.
@samdbmg samdbmg force-pushed the sammg-add-interval-edit branch from 5d5f1d1 to 9de71c9 Compare February 3, 2025 14:20
@philipnbbc philipnbbc self-assigned this Feb 4, 2025
Copy link
Contributor

@philipnbbc philipnbbc left a comment

Choose a reason for hiding this comment

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

A couple comments inline (and a bug found elsewhere) and otherwise LGTM

@@ -61,6 +65,37 @@ async def put_flow(
resp.raise_for_status()


async def get_segments(
Copy link
Contributor

@philipnbbc philipnbbc Feb 4, 2025

Choose a reason for hiding this comment

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

I was going to suggest using the segment generator code but that complicates the use of deque. Also, I note that there is a bug in the function: segment_timerange not in timerange should be segment_timerange in timerange and TimeRange.__contains__ has a bug as TimeRange.from_str("[0:0_1:0)") in TimeRange.eternity() evaluates to False. I'll take a look at fixing the bug and updating the TAMS example.

next_seg = current_seg["list"][0]
next_seg_tr = TimeRange.from_str(next_seg["timerange"]).normalise(edit_rate.numerator,
edit_rate.denominator)
if next_seg_tr.end <= position_in_flow_timeline:
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: is this simpler?

Suggested change
if next_seg_tr.end <= position_in_flow_timeline:
if next_seg_tr.ends_earlier_than_timerange(position_in_flow_timeline):

current_seg["list"].popleft()
continue

next_seg_offset = Timestamp.from_str(next_seg.get("ts_offset", "0:0"))
Copy link
Contributor

Choose a reason for hiding this comment

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

I was slightly confused about what each offset was from the name used. Could this one (which is only used once) be called next_seg_ts_offset and rename the offset in current_seg and other_seg to timeshift? However, it's a personal choice and so I leave it to you to decide what makes sense.

segment_length_remaining = next_seg_tr.end - position_in_flow_timeline
remaining_time_in_cut = next_switch_at - working_time

if (remaining_time_in_cut < segment_length_remaining):
Copy link
Contributor

Choose a reason for hiding this comment

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

There will be some anomalies when doing this and other comparisons if the media rates are not integers (because of rounding errors in some of the calculations using nanoseconds), but I think on balance it isn't helpful to have those complications in this example script.

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