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

codec/decoder/audio: impl Iterator for Audio::decode #74

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

trlim
Copy link

@trlim trlim commented Dec 3, 2016

This is not for merge but for a discussion.

ffmpeg document says "Some decoders may support multiple frames in a single AVPacket" and this is true for APE/WMA Lossless audio streams I have.
(Maybe it can also be true for some "video" decoders)

But there seems to be no way to handle such case because current API doesn't return size of Packet bytes consumed.

This PR is to show what things to be done for this case by implementing an iterator.
For me, a major problem of this implementation is allocating frame::Audio on every iteration.

Want to hear your opinion or suggestion on how we can improve the API.

@trlim
Copy link
Author

trlim commented Dec 3, 2016

Another problem of this implementation is that as packet is borrowed as mutable by iterator, it cannot be borrowed immutable inside the loop.

e if e < 0 => Some(Err(Error::from(e))),
n => {
(*packet).data = (*packet).data.offset(n as isize);
(*packet).size -= n;
Copy link
Owner

Choose a reason for hiding this comment

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

I think it would be better if the packet was borrowed immutably, and then copied over, then these two attributes modified in place in the internal packet.

fn next(&mut self) -> Option<Result<Option<frame::Audio>, Error>> {
unsafe {
if !self.packet.is_empty() {
let mut out = frame::Audio::empty();
Copy link
Owner

Choose a reason for hiding this comment

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

To avoid this, you could create and put the frame inside the AudioFrameIter itself, then you wouldn't need to create a new one on each iteration.

Tae-il Lim added 2 commits December 4, 2016 22:38
Internally makes a copy of AVPacket during the lifetime of the iterator.
let mut avpkt: AVPacket = mem::zeroed();
ptr::copy(packet.as_ptr(), (&mut avpkt) as *mut AVPacket, 1);
avpkt
};
Copy link
Author

Choose a reason for hiding this comment

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

Used unsafe code to make internal copy of AVPacket. This is to avoid packet.clone() which seems to be expensive.

Copy link
Owner

Choose a reason for hiding this comment

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

Can't you just use clone_from?

Copy link
Author

Choose a reason for hiding this comment

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

Because av_copy_packet has some overhead which I think unnecessary but no problem to change it if you prefer.

e if e < 0 => Some(Err(Error::from(e))),
n => {
(*packet).data = (*packet).data.offset(n as isize);
(*packet).size -= n;

if got != 0 {
Some(Ok(Some(out)))
Some(Ok(Some(frame::Audio::wrap(self.frame.as_mut_ptr()))))
} else {
Copy link
Author

Choose a reason for hiding this comment

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

A problem here is the wrapped frame can live longer than the original frame.

@trlim
Copy link
Author

trlim commented Dec 5, 2016

From more investigation, I concluded that allocating a new frame on every iteration is the most safe, simple and cheapest because AVFrame is designed to be heap allocated only and it is what av_frame_ref or av_frame_copy expect, so I'm reverting it.
I also replaced unsafe packet copy with clone_from.

@meh
Copy link
Owner

meh commented Dec 5, 2016

An option to avoid frame reallocations is to give an &mut audio::Frame created by the user to the iterator.

@trlim
Copy link
Author

trlim commented Dec 6, 2016

I tried this but couldn't make it work.

impl Audio {
    pub fn decode_iter<'a, 'b, 'c>(&'a mut self, packet: &'b Packet, frame: &'c mut frame::Audio)
	-> AudioFrameIter<'a, 'b, 'c> {
    ...
}

pub struct AudioFrameIter<'a, 'b, 'c> {
    frame: &'c mut frame::Audio,
    ...
}

impl<'a, 'b, 'c> Iterator for AudioFrameIter<'a, 'b, 'c> {
    type Item = Result<Option<&'c mut frame::Audio>, Error>;
    fn next(&mut self) -> Option<Self::Item> {
        ...
	error[E0495]: cannot infer an appropriate lifetime due to conflicting requirements
	    Some(Ok(Some(self.frame)))
                    ^^^^^^^^^^^^^^^^
        ...
    }
}

I found a discussion on similar problem which I don't fully understand but only gives me a feeling I'm trying something Rust doesn't allow.
Am I trying the method you are telling?

@meh
Copy link
Owner

meh commented Dec 6, 2016

The Item shouldn't contain the frame, since the user already has it.

@trlim
Copy link
Author

trlim commented Dec 7, 2016

Ah yes, but then it will make the iterator less useful and I would prefer not to change current implementation in that case.
I think it might be helpful if we can have additional type which can carry a reference of frame in cheap and safe way.
Current implementation of Dictionary seems to do something similar by Ref and Mut.

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