[FFmpeg-devel] [RFC] Implement support for interplay MVE 0x06, 0x0F and 0x10

[Hein-Pieter van Braam]

I've resubmitted the patch in a more reviewable format here:
http://ffmpeg.org/pipermail/ffmpeg-devel/2017-June/212532.html

having said that:

On Mon, 2017-06-19 at 14:22 +0200, Moritz Barsnick wrote:
> On Sat, Jun 17, 2017 at 19:13:52 +0200, Hein-Pieter van Braam wrote:
> > +            s->pixel_ptr = frame->data[0] + x + y*frame-
> > >linesize[0];

I'll fix this, I copy/pasted this from elsewhere in the file, kind of
sloppy.

> Style: You might want to add spaces around '*'.
> 
> > +            if (! opcode) {
> 
> Style: You should drop the space.

You mean the space between the ! and opcode? The space between if and
opening parenthesis should stay, right?

> 
> > +            if (opcode < 0) {
> > +                off_x = ((unsigned short)opcode - 0xC000) % frame-
> > >linesize[0];
> > +                off_y = ((unsigned short)opcode - 0xC000) / frame-
> > >linesize[0];
> > +                copy_from(s, s->last_frame, frame, off_x, off_y);
> > +            }
> > +            if (opcode > 0) {
> 
> else if?
> 
> > +                    if (opcode < 0) {
> > +                        off_x = ((unsigned short)opcode - 0xC000)
> > % s->cur_decode_frame->linesize[0];
> > +                        off_y = ((unsigned short)opcode - 0xC000)
> > / s->cur_decode_frame->linesize[0];
> > +                        copy_from(s, s->prev_decode_frame, s-
> > >cur_decode_frame, off_x, off_y);
> > +                    }
> > +                    if (opcode > 0) {

I'll fix both of these

> Same here.
> 
> > +    if (s->frame_format == 0x06)
> > +        ipvideo_decode_format_06_opcodes(s, frame);
> > +
> > +    if (s->frame_format == 0x10)
> > +        ipvideo_decode_format_10_opcodes(s, frame);
> > +
> > +    if (s->frame_format == 0x11)
> > +        ipvideo_decode_format_11_opcodes(s, frame);
> 
> Unless ipvideo_decode_format_NN_opcodes() modifies s->frame_format,
> which doesn't seem to be the case, this is either if/else if/else if,
> or switch/case.

This got replaced with a switch case in the new patchset

> Cheers,
> Moritz

Thanks for the review!