Skip to content

AudioFrame.copyfrom() should move the internal used_size marker #190

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

Closed
microbit-carlos opened this issue Apr 9, 2024 · 4 comments
Closed
Assignees
Labels
bug Something isn't working
Milestone

Comments

@microbit-carlos
Copy link
Contributor

microbit-carlos commented Apr 9, 2024

Using the latest version (at the time of writing) of the recording & playback branch: 0b06914
Hex: https://github.com/microbit-foundation/micropython-microbit-v2/actions/runs/8416237764?pr=163

>>> a = microphone.record(100)
>>> b = audio.AudioFrame(100)
>>> b.copyfrom(a)
>>> # This plays a short audio
>>> audio.play(a)
>>> # This does not
>>> audio.play(b)
>>> # And the  sound data is there
>>> for i in range(len(a)):
...     assert a[i] == b[i]
... 
>>> 

If we manually edit the last byte in b it then does play a short clip:

>>> b[len(b) - 1] = 255
>>> audio.play(b)
>>> 
@microbit-carlos microbit-carlos added the bug Something isn't working label Apr 9, 2024
@microbit-carlos microbit-carlos added this to the 2.2.0-beta.1 milestone Apr 9, 2024
@microbit-carlos microbit-carlos changed the title AudioFrame copyfrom() should move the internal used_size marker AudioFrame.copyfrom() should move the internal used_size marker Apr 9, 2024
@dpgeorge
Copy link
Collaborator

Copying comment #163 (comment) here:

This needs discussion. copyfrom() actually allows copying from any object with the buffer protocol (eg bytes, memoryview) and they won't necessarily have the used_size attribute.

This method should probably just set used_size to the total bytes copied fro the buffer-like object. The only issue there is copying from another AudioFrame it will copy the total allocated size, not the used_size, of the other AudioFrame.

@microbit-carlos
Copy link
Contributor Author

microbit-carlos commented Apr 26, 2024

Yes, I think AudioFrame.copyfrom() should set used_size to to the last byte copied from the input argument (any buffer-like object).

So, if we start with a long AudioFrame that has data all the way to the end, and then we copy a short buffer into the AudioFrame, then we would end up with the used_size marker at length of the small buffer.
But I think this makes sense only if we also provide accessors to the used_marker (#196), otherwise there is "magic" that cannot be changed without odd workarounds (like assigning to itself the last indexable value, e.g audio_frame[len(audio_frame) - 1] = audio_frame[len(audio_frame) - 1] vs audio_frame.set_marker(len(audio_frame))).

The question would be, do we special-case copying from another AudioFrame?
We'd have a few options, for all of these we can consider the following set up code:

af_long = audio.AudioFrame(duration=5000)
af_short = audio.AudioFrame(duration=2000)
microphone.record_into(af_short, wait=False)
sleep(1000)    # Record for only 1 second, af_short is only half filled
microphone.stop_recording()
  1. The AudioFrame is fully copied, including the used_size value
    af_long.copyfrom(af_short)
    audio.play(af_long) # Plays for 1 second
  2. The AudioFrame is fully copied, the used_marker placed at size of the input argument
    af_long.copyfrom(af_short)
    audio.play(af_long) # Plays for 2 second, 1st second audio data, 2nd second empty
    af_long.set_position(af_short.get_position())
    audio.play(af_long) # Plays for 1 second
  3. The AudioFrame is only copied until the used_marker
    af_long.copyfrom(af_short)
    audio.play(af_long) # Plays for 1 second

In my opinion, for a user option 1. and 3. make most sense for easy-of-use. Option 2. is not too bad, as it makes sense if you already know about the marker, however I think it's a lot more likely that the users would want 1 second playback instead of 2, so making that default makes sense.

From options 1. and 3. , I'm a lot more inclined on option 1., because option 3. limiting how much data is copied doesn't feel quite right.


This might also need to be discussed with #194 in mind, as one of the suggestions is to add an extra argument to have AudioFrame.copyfrom(buffer, index).

af_long = AudioFrame(duration=5000)
af_short = AudioFrame(duration=2000) 
while True:
    if button_a.is_pressed():
        microphone.record_into(af_short, wait=False)
        while button_a.is_pressed():
            pass
        microphone.stop()
        af_long.copyfrom(af_short, index=af_long.get_position())
    if button_b.is_pressed():
        audio.play(large_af)
    sleep(200)

So in these cases we would need to make sure that when calculating the user_size position, it also needs to be offset from the index point (from AudioFrame.copyfrom(buffer, index)).

@microbit-carlos
Copy link
Contributor Author

This can be separated in two parts:

  1. We are in agreement that copyfrom() should move the marker based on the size of the data copied
  2. We need to consider if we want to special case when copying from an AudioFrame that might have a marker not at the end of the buffer

So, 1. can be implemented already, and 2 will be considered as part of the new proposal:

@microbit-carlos
Copy link
Contributor Author

We've changed the implementation and the new audio objects do not have an "internal marker":

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants