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

Enhancements for precise A/V sync #490

Merged
merged 2 commits into from
Nov 2, 2018
Merged

Conversation

dbbnrl
Copy link
Contributor

@dbbnrl dbbnrl commented May 20, 2018

I've been working on some code that captures audio and attempts to precisely synchronize it with PiCamera video. I've been having a lot of trouble getting precise control over the number of seconds written by PiCameraCircularIO.copy_to(). This PR attempts to address this issue, and also adds some minor features I've found useful.

The problem is in the handling of frames that are split into multiple chunks (where only the last chunk is marked 'complete'). When chunks are delivered to PiVideoEncoder._callback_write(), only the first chunk of a frame gets a timestamp (I believe this is a hardware behavior). However, in PiCameraDequeueHack.append(), frame metadata is only saved for the last chunk of a frame. The net result is that split frames never have timestamp data visible to PiCameraCircularIO._find_seconds(), causing it to skip over them.

This PR makes the following changes:

  • PiVideoEncoder._callback_write() replicates the timestamp from the first chunk to all subsequent chunks of a given frame. This fixes the behavior of _find_seconds().
  • split_recording() now returns the PTS timestamp of the split point. This is not required to fix the bug, but is useful for precise sync with audio streams.
  • Similarly, copy_to() now returns the start and end timestamp of the data that was written out.
  • Some of the hackiness of PiCameraDequeueHack is moved into PiCameraCircularIO. Not strictly necessary, but convenient for some other feature additions I'll put in a future PR.

Assign timestamp to all chunks of a frame, not just the first.
Return PTS of split point.
@julesrenaud
Copy link

Hi! I'm very interested by this implementation. I'm curious, what kind of mic are you using for your testing? One of those diminutive usb mics?

@dbbnrl
Copy link
Contributor Author

dbbnrl commented May 22, 2018

The mic I have been playing with is a USB mic, but I wouldn't say it's particularly small. I haven't really paid attention to that, just been trying to get the functionality working. I could try to figure it out when I get home but honestly it's nothing special.

@waveform80
Copy link
Owner

Fantastic! I need to have a bit of a play with this before merging it, but from a quick skim this looks good (and preserves backward compatibility very nicely; i.e. I've no issue adding a return value to methods which previously just returned None).

Note to self: update documentation to document return values.

@waveform80 waveform80 added this to the 1.14 milestone Sep 29, 2018
@waveform80 waveform80 self-assigned this Sep 29, 2018
@waveform80
Copy link
Owner

Don't worry about the merge conflict; I'll sort that out while I merge this (just occurred because I merged #435 first). Having had a quick play with this I like the idea but I am going to make a couple of changes:

  • I definitely like the idea of copy_to returning something indicating the range it's selected; it's useful and it doesn't break backwards compatibility
  • I don't see a particular reason that result should be restricted to being returned when seconds is specified; i.e. whatever search criteria are used (including frames from Add support for frames in PiCameraCircularIO.copy_to() #435), the range should be returned
  • The timestamp is likely most useful, but it's probably more useful to just return the frame meta-data, i.e. instead of returning start_ts, end_ts I'm going to tweak it to return start_frame, end_frame (which will contain the timestamps obviously)

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

Successfully merging this pull request may close these issues.

3 participants