On Fri, 15 Apr 2016, Jan Sebechlebsky wrote:
On 04/15/2016 02:28 AM, Marton Balint wrote:
On Thu, 14 Apr 2016, Marton Balint wrote:
On Tue, 12 Apr 2016, sebechlebsky...@gmail.com wrote:
From: Jan Sebechlebsky <sebechlebsky...@gmail.com>
Calling close_slave in case error is to be returned from open_slave
will free allocated resources.
Since failure can happen before bsfs array is initialized,
close_slave must check that bsfs is not NULL before accessing
tee_slave->bsfs[i] element.
Slave muxer expects write_trailer to be called if it's
write_header suceeded (so resources allocated in write_header
are freed). Therefore if failure happens after successfull
write_header call, we must ensure that write_trailer of
that particular slave is called.
Hmm, I guess you are right, I see no other way freeing resources allocated
in write_header then calling write_trailer. It does make the code a bit
more complex, but I don't really see a way to make it more simple.
So this looks good to me. Nicolas, any ideas improving this?
Actually I have given this some additional thought, and by using a new
per-slave variable to keep the information if the header was written or
not, I think your patch can be simplifed, also close_slave can be changed
so it will write the trailer if necessary, in fact, the write_trailer
function can use it as well.
I have modified your patch (see attached), could you please test/review it
and check if I missed anything? I hope you don't mind, this kind of
collaborative work is not that common in ffmpeg, but in this case it seemed
easier moving those few lines around than describing what I wanted.
Thanks,
Marton
Hello Marton,
I'm ok with it, you're right it is more elegant this way. I've tested it and
it seems allright. I've recreated the last patch on top of these changes and
I'm sending it in attachment (and I am also cc-ing this mail to Nicolas, so
he can review the patches).
[...]
@@ -423,8 +486,9 @@ static int tee_write_header(AVFormatContext *avf)
return 0;
fail:
- for (i = 0; i < nb_slaves; i++)
+ for (i = 0; i < nb_slaves; i++) {
av_freep(&slaves[i]);
+ }
This seems a no-op change.
[...]
static int tee_write_trailer(AVFormatContext *avf)
{
TeeContext *tee = avf->priv_data;
+ AVFormatContext *avf2;
int ret_all = 0, ret;
unsigned i;
for (i = 0; i < tee->nb_slaves; i++) {
- if ((ret = close_slave(&tee->slaves[i])) < 0)
- if (!ret_all)
+ if (!(avf2 = tee->slaves[i].avf))
+ continue;
+ if ((ret = close_slave(&tee->slaves[i])) < 0) {
+ ret = tee_process_slave_failure(avf2, i, ret);
+ if (!ret_all && ret < 0)
ret_all = ret;
+ }
}
+ close_slaves(avf);
I guess you don't need close_slaves here, because you already closed all
slaves.
Regards,
Marton
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
