Corrupt decoding :(

[Paul-Arnold]

Corrupt decoding :(

[Paul-Arnold]

For further info, I've just tested this update using 30+ mpeg files which previously suffered massive amounts of corruption and they all decode correctly :)

[jrdennisoss]

I FINALLY had a few cycles to take a look at this over lunch today... THANK YOU Paul for a MUCH more in-spec solution than the hack I earlier suggested... Yes I confirm this works on my setup and actually fixes another issues I had too! 😄

I ended up deviating slightly from what you had:

int plm_buffer_peek_non_zero(plm_buffer_t *self, int bit_count) {
    if (!plm_buffer_has(self, bit_count)) {
        return FALSE;
    }

    int val = plm_buffer_read(self, bit_count);
    self->bit_index -= bit_count;
    return val != 0;
}

void plm_video_decode_slice(plm_video_t *self, int slice) {
    self->slice_begin = TRUE;
    self->macroblock_address = (slice - 1) * self->mb_width - 1;

    // Reset motion vectors and DC predictors
    self->motion_backward.h = self->motion_forward.h = 0;
    self->motion_backward.v = self->motion_forward.v = 0;
    self->dc_predictor[0] = 128;
    self->dc_predictor[1] = 128;
    self->dc_predictor[2] = 128;

    self->quantizer_scale = plm_buffer_read(self->buffer, 5);

    // Skip extra
    while (plm_buffer_read(self->buffer, 1)) {
        plm_buffer_skip(self->buffer, 8);
    }

    do {
        plm_video_decode_macroblock(self);
    } while (
        self->macroblock_address < self->mb_size - 1 &&
        plm_buffer_peek_non_zero(self->buffer, 23)
    );
}

The logic is exactly the same as yours, but I renamed things to better match what ISO/IEC 11172-2 says as its logic is pretty explicit: do ... while the next 23 bits are not zero. Then only after the do/while condition is the start code checked.

[Zero3K]

MAME? Is that the arcade emulator?

[Paul-Arnold]

Yes. I’ve some games which need mpeg1. Can’t get pl_mpeg to work though:(

…

On 17 Feb 2022, at 15:04, Bryan Kirk ***@***.***> wrote:  MAME? Is that the arcade emulator? — Reply to this email directly, view it on GitHub, or unsubscribe. Triage notifications on the go with GitHub Mobile for iOS or Android. You are receiving this because you authored the thread.

[jrdennisoss]

I actually fixed this problem in the fork of PL_MPEG I am using for the DOSBox ReelMagic project. Here is the change I made: jrdennisoss/dosboxrm@712f309

What is happening is that sometimes a slice boundary is blown through because the ISO spec allows for a bit of extra '0' padding at the end, but the current version of PL_MPEG is not quite handling this right. I made a patch which guards the slice boundary based on the known last macroblock address for that specific slice. I tested with your MPG file you posted above and this seems to do the trick.

[jrdennisoss]

I created a pull request which should fix this: #26

This awesome library has proved to be invaluable to the project I have been working on, I figure contributing something back is the very least I should do 😄

[Paul-Arnold]

That’s brilliant, thanks for reporting the fix.
I started to look but my lack of knowledge on mpeg and trying to understand how it should work meant I quickly got lost :(
Unfortunately I switched to another library for my current project but will investigate the use of this one again to see if it’s worth trying to switch back.

Thanks again
Paul

[livid123]

but this patch not working on bjork-all-is-full-of-love.mpg

[jrdennisoss]

Oh crap... Good catch @livid123 I'll close the pull request I opened until I fully understand this... Probably need to implement something that truly finds the slice boundary instead of assuming that there is only one macroblock row per slice.

[Paul-Arnold]

I've looked at what my working decoder does and it continues processing macro blocks until the next 23 bits are 0.
Ie, it doesn't abort on finding a start code but on a possible start code. It doesn't check if the 23 bits are aligned.
A quick test changing plm_buffer_no_start_code() to the following and it appears to work on bjork and my files.
Can someone confirm it works on others too ?

int plm_buffer_no_start_code(plm_buffer_t *self) {
int code;
if (!plm_buffer_has(self, (23))) {
return FALSE;
}

code = plm_buffer_read(self, 23);

self->bit_index -= 23;//rewind buffer pointer as we're only peeking

return code != 0 ;

}

Edit: In addition to the above the macro processing loop can become as follows
do {
plm_video_decode_macroblock(self);
} while ( plm_buffer_no_start_code(self->buffer) );

[Zero3K]

Will you make a new PR soon since you have now fixed the issues that @livid123 and you have found?

[jrdennisoss]

Sure thing: #28

[Paul-Arnold]

Thanks for doing the nicer named solution Jon. Great that it fixes your other issue too.

I simply compared your original solution with the decoder I'm using (which is based on the early Berkeley code), doubt I'd have found it otherwise.

[phoboslab]

Outstanding work everyone! Thank you so much :)