Hi, On Tue, Jun 7, 2011 at 10:35 AM, John Stebbins <[email protected]> wrote: > On 06/02/2011 05:44 PM, John Stebbins wrote: >> >> On 06/02/2011 04:59 PM, Ronald S. Bultje wrote: >>> >>> Hi John, >>> >>> On Thu, Jun 2, 2011 at 4:50 PM, John Stebbins<[email protected]> >>> wrote: >>>> >>>> On 06/02/2011 04:42 PM, Ronald S. Bultje wrote: >>>>> >>>>> Hi John, >>>>> >>>>> On Thu, Jun 2, 2011 at 4:39 PM, John Stebbins<[email protected]> >>>>> wrote: >>>>>> >>>>>> On 06/02/2011 03:31 PM, Ronald S. Bultje wrote: >>>>>>> >>>>>>> On Thu, Jun 2, 2011 at 3:29 PM, John >>>>>>> Stebbins<[email protected]> >>>>>>> wrote: >>>>>>>> >>>>>>>> Fair enough. There appear to be some cases remaining where >>>>>>>> av_malloc >>>>>>>> is >>>>>>>> called with a size of 0. I'll set a break point and find out where >>>>>>>> the >>>>>>>> request is coming from. >>>>>>> >>>>>>> Thanks, if you can just show me a backtrace (I just run commands and >>>>>>> set a if (size == 0) abort() in av_malloc), I'll gladly accept the >>>>>>> fix. >>>>>>> >>>>>>> >>>>>> The first occurrence of the problem that I find is in vc1_decode_init. >>>>>> avctx->mb_stride gets set (from avctx->width) when >>>>>> ff_msmpeg4_decode_init >>>>>> is called. But at the point ff_msmpeg4_decode_init is called >>>>>> avctx->width >>>>>> hasn't been set yet (i.e. it is 0). width gets set in >>>>>> vc1_decode_sequence_header which is called after >>>>>> ff_msmpeg4_decode_init. >>>>>> Since width is 0, mb_stride is 0 and av_malloc gets called with 0. >>>>>> >>>>>> I don't know exactly how to untangle this because I don't know the >>>>>> dependencies between these initialization functions. Can anyone make >>>>>> any >>>>>> suggestions? >>>>> >>>>> Feel free to send me a file that triggers the problem and I'll happily >>>>> fix it for you. >>>>> >>>>> >>>> I think this would take more than just a sample file to reproduce. I am >>>> reproducing this with HandBrake which has it's own TS demux. So we are >>>> not >>>> using libavformat in this case and are allocating and initializing >>>> AVCodecContext, initializing extradata, and calling avcodec_open >>>> ourselves. >>>> >>>> We've always noticed a little weirdness with vc1 in this bit of code. >>>> We >>>> have to call avcodec_open twice to get it to initialize properly due to >>>> this >>>> order of operations problem in vc1_decode_init. >>> >>> Which allocation specifically? >> >> There is a collection of av_malloc's in vc1_decode_init >> /* Allocate mb bitplanes */ >> v->mv_type_mb_plane = av_malloc(s->mb_stride * s->mb_height); >> v->direct_mb_plane = av_malloc(s->mb_stride * s->mb_height); >> v->acpred_plane = av_malloc(s->mb_stride * s->mb_height); >> v->over_flags_plane = av_malloc(s->mb_stride * s->mb_height); >> >> v->n_allocated_blks = s->mb_width + 2; >> v->block = av_malloc(sizeof(*v->block) * v->n_allocated_blks); >> v->cbp_base = av_malloc(sizeof(v->cbp_base[0]) * 2 * s->mb_stride); >> v->cbp = v->cbp_base + s->mb_stride; >> v->ttblk_base = av_malloc(sizeof(v->ttblk_base[0]) * 2 * s->mb_stride); >> v->ttblk = v->ttblk_base + s->mb_stride; >> v->is_intra_base = av_malloc(sizeof(v->is_intra_base[0]) * 2 * >> s->mb_stride); >> v->is_intra = v->is_intra_base + s->mb_stride; >> v->luma_mv_base = av_malloc(sizeof(v->luma_mv_base[0]) * 2 * >> s->mb_stride); >> v->luma_mv = v->luma_mv_base + s->mb_stride; >> >> /* allocate block type info in that way so it could be used with >> s->block_index[] */ >> v->mb_type_base = av_malloc(s->b8_stride * (s->mb_height * 2 + 1) + >> s->mb_stride * (s->mb_height + 1) * 2); >> >>> I fixed MPV_common_init(), which is >>> doing most allocs here. The correct thing to do is to re-run >>> MPV_common_init() after the sequence header has been read, can you >>> experiment with that and see if that fixes it? You may have to >>> MPV_common_end() first to prevent memory leaks. >> >> The actual call chain to get to MPV_common_init is >> vc1_decode_init->ff_msmpeg4_decode_init->ff_h263_decode_init->MPV_common_init. >> It is necessary to call ff_msmpeg4_decode init again instead of >> MPV_common_init directly because ff_msmpeg4_decode init uses mb_height after >> MPV_common_init sets it. Here's a patch that shows what I did. With this >> patch applied, decoding works, I don't have to call avcodec_open twice, and >> I no longer get av_malloc(0). >> >> I've test this with mpeg2, h.264, vc-1 video and ac3, dts, truehd, dts-hd >> audio codecs in TS files (basically a bunch of BD m2ts files). So it's >> looking pretty solid. >> >> > Are there any plans for resolving this in the not too distant future? I'm > not trying to rush anyone. Just making plans for how I should proceed with > the HandBrake code that triggers this.
My apologies. I was wondering if this triggers for all vc1 files, or just wvp2/wmvp ones (this is where read_sequence_header sets size, IIRC). Alternatively, maybe we can compare w/h before and after read_sequence_header so we don't needlessly reinitialize (it is quite heavy, so prefer to prevent it if not necessary). But the idea of the patch is good, I'm OK with applying it with the above reoslved. Ronald _______________________________________________ libav-devel mailing list [email protected] https://lists.libav.org/mailman/listinfo/libav-devel
