[FFmpeg-devel] [PATCH] h264 parallelized, (was: Parallelized h264 proof-of-concept)

[Michael Niedermayer]

Hi

On Sat, Jun 16, 2007 at 11:03:22PM +0200, Andreas ?man wrote:
> Hi
> 
> Michael Niedermayer wrote:
> > Hi
> > 
> > On Fri, Jun 15, 2007 at 10:10:54PM +0200, Andreas ?man wrote:
> >> Andreas ?man wrote:
> >>> Hi
> >>>
> >>> Michael Niedermayer wrote:
> >>>> Hi
> >>>>
> >>>>
> >>>> av_free() + av_malloc() or pass an argument to MPV_common_init()
> >>> I'll extend MPV_common_init() with an additional argument then.
> >>>
> >>>>> static void filter_mb_fast( H264Context *h, int mb_x, int mb_y, uint8_t 
> >>>>> *img_y, uint8_t *img_cb, uint8_t *img_cr, unsigned int linesize, 
> >>>>> unsigned int uvlinesize);
> >>>>> +static void execute_decode_slices(H264Context *h, int reset);
> >>>> cant you order the new functions so as to avoid that?
> >>> Hm, not really. decode_slice_header() needs to be able to
> >>> fire off any pending slices in case the deblocking-type changes
> >>> within a frame. (AFAIK this is valid according to the specs,
> >>> perhaps I'm wrong?)
> >>>
> >> Okay, here are the finalized patches.
> >>
> >> #1 - Extend MPV_common_init() with an addition arg for context size
> >>      when doing multi threading.
> > 
> > hmm, seeing the patch, i think i would prefer some simpler solution,
> 
> I tend to agree, this is a bit too intrusive. But i thought
> you were okay with it.

i thought so too until i saw the patch


> 
> > maybe adding the size to MpegEncContext? or even better adding
> > thread_context[] to H264Context, this would also avoid the casts to
> > H264Context
> 
> Personally, i like the idea of having mpegvideo.c do most of the
> thread context setup. In the long run i guess it will be less
> code duplication and such things.
> 
> How about just letting MPV_common_init use ctx->codec->priv_data_size
> for malloc'ing the contexts? Or is that too inflexible, in that case
> I tend to favor adding "priv_data_size" to MpegEncContext

it still would be allocating H264Contexts into a MpegEncContext pointer
somehow this doesnt feel correct


> 
> 
> >> #4 - Slice level parallelism for deblocking type 0 and 2
> >>
> >> regression tests passes
> > 
> > regression tests ? theres no h.264 regression test ...
> > i assume you mean you tested this on several h.264 streams and the
> > output is binary identical ...
> 
> It was more to make sure that i didn't screw anything else up when
> doing the MPV_common_init() thingie.
> 
> I've tested with all the h264 samples from mphq and they
> are all md5sum identical. I've also got a few off-list replies
> with people testing the code whom reported back various issues
> which I've fixed (Thanks to all of you).

in case you or anyone else wants to do more extensive tests on any change
to the h.264 decoder (no iam not suggesting that you do for the this patch)
see http://ftp3.itu.ch/av-arch/jvt-site/draft_conformance/
for some h.264 streams
these test many more cases than normal real live h264 streams


[...]
> >> +
> >> +    h->max_contexts = avctx->thread_count > 0 ? avctx->thread_count : 1;
> > 
> > i think thread_count must be >0
> 
> Hmm, set by whom? The application certainly doesn't need to and AFAIK
> lavc doesn't peg it up to 1 either.

hmm just leave the check in there

[...]

> > 
> > 
> > [...]
> >>          case NAL_DPA:
> >> -            init_get_bits(&s->gb, ptr, bit_length);
> >> -            h->intra_gb_ptr=
> >> -            h->inter_gb_ptr= NULL;
> >> -            s->data_partitioning = 1;
> >> +             init_get_bits(&hx->s.gb, ptr, bit_length);
> >> +             hx->intra_gb_ptr=
> >> +             hx->inter_gb_ptr= NULL;
> >> +             hx->s.data_partitioning = 1;
> >>  
> >> -            if(decode_slice_header(h) < 0){
> >> +            if(decode_slice_header(hx, h) < 0){
> >>                  av_log(h->s.avctx, AV_LOG_ERROR, "decode_slice_header error\n");
> >>              }
> > 
> > indention is wrong here
> 
> I'll look into it. btw parts of decode_nal_units() in svn is incorrectly
> indented, wouldn't it be wise to fix that up? Should i send a patch?

yes patch welcome

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

It is dangerous to be right in matters on which the established authorities
are wrong. -- Voltaire
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20070617/c85aaa10/attachment.pgp>