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

Add proper error handling, make sure sink cannot panic #138

Merged
merged 3 commits into from Sep 28, 2018

Conversation

Projects
None yet
3 participants
@Manishearth
Member

Manishearth commented Sep 27, 2018

Not sure if the decoder side needs this

r? @ferjm

let appsrc = appsrc.downcast::<AppSrc>().map_err(|_| ())?;
let appsrc = gst::ElementFactory::make("appsrc", None)
.ok_or(BackendError::ElementCreationFailed("appsrc"))?;
let appsrc = appsrc.downcast::<AppSrc>().map_err(|_| BackendError::DowncastingFailed)?;

This comment has been minimized.

@sdroege

sdroege Sep 28, 2018

Contributor

This is impossible to fail in this case. You create an appsrc so it will be possible to cast it

let audio_info = gst_audio::AudioInfo::new(
gst_audio::AUDIO_FORMAT_F32,
sample_rate as u32,
channels.into(),
).build()
.ok_or(())?;
.ok_or(BackendError::AudioInfoFailed)?;

This comment has been minimized.

@sdroege

sdroege Sep 28, 2018

Contributor

This also can't possibly fail. The only way how this can fail if incompatible other settings (e.g. conflicting channel positions) are given

This comment has been minimized.

@Manishearth

Manishearth Sep 28, 2018

Member

This feels like it's possible to cause in a refactoring, so I'll leave this error in

gst::Element::link_many(&[&appsrc, &resample, &convert, &sink]).map_err(|_| ())?;
.map_err(|_| BackendError::PipelineFailed)?;
gst::Element::link_many(&[&appsrc, &resample, &convert, &sink])
.map_err(|_| BackendError::PipelineFailed)?;

This comment has been minimized.

@sdroege

sdroege Sep 28, 2018

Contributor

You maybe want to take the &'static str out of the error and store it in PipelineFailed

let appsrc = appsrc.downcast::<AppSrc>().map_err(|_| ())?;
let appsrc = gst::ElementFactory::make("appsrc", None)
.ok_or(BackendError::ElementCreationFailed("appsrc"))?;
let appsrc = appsrc.downcast::<AppSrc>().map_err(|_| BackendError::DowncastingFailed)?;
Ok(Self {
pipeline: gst::Pipeline::new(None),
appsrc: Arc::new(appsrc),

This comment has been minimized.

@sdroege

sdroege Sep 28, 2018

Contributor

Why is the appsrc not put into the pipeline here already?

Also generally it seems like for all pipelines you're not watching their gst::Bus for messages, especially not for error messages. It would be good to add some code for that so that you can handle errors (and later possibly other things) asynchronously while the pipeline is running

This comment has been minimized.

@ferjm

ferjm Sep 28, 2018

Member

Also generally it seems like for all pipelines you're not watching their gst::Bus for messages, especially not for error messages. It would be good to add some code for that so that you can handle errors (and later possibly other things) asynchronously while the pipeline is running

Yeah, we'll need that for #125, so I can take care of this for the audio sink and the player there as well.

@ferjm

Thank you for starting this! I had a half backed patch with something similar, but you beat me :)

I think we need this for the audio decoder (i.e. #125) and the player as well. But we can add it in follow ups.

Looks good, with @sdroege's feedback addressed and the question about the separate dummy backend crate answered.

@@ -2,6 +2,8 @@ extern crate ipc_channel;
#[macro_use]
extern crate serde_derive;
extern crate servo_media_audio;

This comment has been minimized.

@ferjm

ferjm Sep 28, 2018

Member

Could we move the implementation of the dummy backend to a separate crate so we don't make servo_media_player depend on servo_media_audio?

This comment has been minimized.

@Manishearth

Manishearth Sep 28, 2018

Member

It's hard because servo-media-audio needs the dummy backend.

However I can make the PlayerBackend trait completely separate. I'll try something.

@Manishearth Manishearth force-pushed the Manishearth:errors branch from b25f2a0 to 2306b55 Sep 28, 2018

@Manishearth Manishearth force-pushed the Manishearth:errors branch from 2306b55 to 9419d4f Sep 28, 2018

@Manishearth

This comment has been minimized.

Member

Manishearth commented Sep 28, 2018

Addressed

@Manishearth Manishearth merged commit e68840f into servo:master Sep 28, 2018

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@Manishearth Manishearth deleted the Manishearth:errors branch Sep 28, 2018

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