[PATCH] MJPEG: emulate EOI also on two consecutive SOI.

[Reimar Döffinger]

Fixes issue #362.

Signed-off-by: Reimar Döffinger <***@gmx.de>
---
libavcodec/mjpegdec.c | 4 ++++
1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/libavcodec/mjpegdec.c b/libavcodec/mjpegdec.c
index 6331e3d..57ce821 100644
--- a/libavcodec/mjpegdec.c
+++ b/libavcodec/mjpegdec.c
@@ -1430,6 +1430,10 @@ int ff_mjpeg_decode_frame(AVCodecContext *avctx,

s->restart_count = 0;
/* nothing to do on SOI */
+ if (s->got_picture) {
+ av_log(avctx, AV_LOG_WARNING, "EOI missing, emulating\n");
+ goto eoi_parser;
+ }
break;
case DQT:
ff_mjpeg_decode_dqt(s);

[Michael Niedermayer]

Post by Reimar Döffinger
Fixes issue #362.
---
libavcodec/mjpegdec.c | 4 ++++
1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/libavcodec/mjpegdec.c b/libavcodec/mjpegdec.c
index 6331e3d..57ce821 100644
--- a/libavcodec/mjpegdec.c
+++ b/libavcodec/mjpegdec.c
@@ -1430,6 +1430,10 @@ int ff_mjpeg_decode_frame(AVCodecContext *avctx,
s->restart_count = 0;
/* nothing to do on SOI */
+ if (s->got_picture) {
+ av_log(avctx, AV_LOG_WARNING, "EOI missing, emulating\n");
+ goto eoi_parser;

should that not jump to 3 lines earlier to emulate EOI fully ?

[...]

[Reimar Döffinger]

Post by Michael Niedermayer

Post by Reimar Döffinger
Fixes issue #362.
---
libavcodec/mjpegdec.c | 4 ++++
1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/libavcodec/mjpegdec.c b/libavcodec/mjpegdec.c
index 6331e3d..57ce821 100644
--- a/libavcodec/mjpegdec.c
+++ b/libavcodec/mjpegdec.c
@@ -1430,6 +1430,10 @@ int ff_mjpeg_decode_frame(AVCodecContext *avctx,
s->restart_count = 0;
/* nothing to do on SOI */
+ if (s->got_picture) {
+ av_log(avctx, AV_LOG_WARNING, "EOI missing, emulating\n");
+ goto eoi_parser;

should that not jump to 3 lines earlier to emulate EOI fully ?

No idea, should it?
The existing code to emulate EOI at the end of a buffer jumps to exactly
that location.
restart_interval is 0 anyway after SOI, so that break will not trigger
in the normal case, and neither should it I think.
Should the buggy_avid case skip the cur_scan = 0 code?
Should cur_scan possibly be reset on every SOF anyway, or rather instead?

[Michael Niedermayer]

Post by Reimar Döffinger

Post by Michael Niedermayer

Post by Reimar Döffinger
Fixes issue #362.
---
libavcodec/mjpegdec.c | 4 ++++
1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/libavcodec/mjpegdec.c b/libavcodec/mjpegdec.c
index 6331e3d..57ce821 100644
--- a/libavcodec/mjpegdec.c
+++ b/libavcodec/mjpegdec.c
@@ -1430,6 +1430,10 @@ int ff_mjpeg_decode_frame(AVCodecContext *avctx,
s->restart_count = 0;
/* nothing to do on SOI */
+ if (s->got_picture) {
+ av_log(avctx, AV_LOG_WARNING, "EOI missing, emulating\n");
+ goto eoi_parser;

should that not jump to 3 lines earlier to emulate EOI fully ?

No idea, should it?
The existing code to emulate EOI at the end of a buffer jumps to exactly
that location.
restart_interval is 0 anyway after SOI, so that break will not trigger
in the normal case, and neither should it I think.
Should the buggy_avid case skip the cur_scan = 0 code?

I dont think it should skip it

Post by Reimar Döffinger
Should cur_scan possibly be reset on every SOF anyway, or rather instead?

i think cur_scan should be reset and it possibly needs to be checked
somewhere, the code using it looks a bit risky

[...]

--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Old school: Use the lowest level language in which you can solve the problem
conveniently.
New school: Use the highest level language in which the latest supercomputer
can solve the problem without the user falling asleep waiting.

[Reimar Döffinger]

Post by Michael Niedermayer

Post by Reimar Döffinger

Post by Michael Niedermayer

Post by Reimar Döffinger
Fixes issue #362.
---
libavcodec/mjpegdec.c | 4 ++++
1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/libavcodec/mjpegdec.c b/libavcodec/mjpegdec.c
index 6331e3d..57ce821 100644
--- a/libavcodec/mjpegdec.c
+++ b/libavcodec/mjpegdec.c
@@ -1430,6 +1430,10 @@ int ff_mjpeg_decode_frame(AVCodecContext *avctx,
s->restart_count = 0;
/* nothing to do on SOI */
+ if (s->got_picture) {
+ av_log(avctx, AV_LOG_WARNING, "EOI missing, emulating\n");
+ goto eoi_parser;

should that not jump to 3 lines earlier to emulate EOI fully ?

No idea, should it?
The existing code to emulate EOI at the end of a buffer jumps to exactly
that location.
restart_interval is 0 anyway after SOI, so that break will not trigger
in the normal case, and neither should it I think.
Should the buggy_avid case skip the cur_scan = 0 code?

I dont think it should skip it

What is it supposed to do anyway?
The comment says
/* if restart period is over process EOI */
However the comment is:
if ((s->buggy_avid && !s->interlaced) || s->restart_interval)
Which will go to EOI when the restart feature is enabled,
not when the restart period is over.

[Reimar Döffinger]

Post by Michael Niedermayer

Post by Reimar Döffinger
Fixes issue #362.
---
libavcodec/mjpegdec.c | 4 ++++
1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/libavcodec/mjpegdec.c b/libavcodec/mjpegdec.c
index 6331e3d..57ce821 100644
--- a/libavcodec/mjpegdec.c
+++ b/libavcodec/mjpegdec.c
@@ -1430,6 +1430,10 @@ int ff_mjpeg_decode_frame(AVCodecContext *avctx,
s->restart_count = 0;
/* nothing to do on SOI */
+ if (s->got_picture) {
+ av_log(avctx, AV_LOG_WARNING, "EOI missing, emulating\n");
+ goto eoi_parser;

should that not jump to 3 lines earlier to emulate EOI fully ?

I sent a patch to move resetting cur_scan.
After that I think it is the right place to go to.
The if as far as I can tell is there to avoid a warning for the buggy
case when the EOI ends up at the wrong place. Or something like that.
In either way if we encounter an SOI we really want the EOI code to
be run, not skipped like that condition does.
Though it might help to have that avid sample (and any others) that
condition is supposed to check.
So in short, for now I still think this proposed patch is doing the
right thing.

[Michael Niedermayer]

Post by Reimar Döffinger

Post by Michael Niedermayer

Post by Reimar Döffinger
Fixes issue #362.
---
libavcodec/mjpegdec.c | 4 ++++
1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/libavcodec/mjpegdec.c b/libavcodec/mjpegdec.c
index 6331e3d..57ce821 100644
--- a/libavcodec/mjpegdec.c
+++ b/libavcodec/mjpegdec.c
@@ -1430,6 +1430,10 @@ int ff_mjpeg_decode_frame(AVCodecContext *avctx,
s->restart_count = 0;
/* nothing to do on SOI */
+ if (s->got_picture) {
+ av_log(avctx, AV_LOG_WARNING, "EOI missing, emulating\n");
+ goto eoi_parser;

should that not jump to 3 lines earlier to emulate EOI fully ?

I sent a patch to move resetting cur_scan.
After that I think it is the right place to go to.
The if as far as I can tell is there to avoid a warning for the buggy
case when the EOI ends up at the wrong place. Or something like that.
In either way if we encounter an SOI we really want the EOI code to
be run, not skipped like that condition does.
Though it might help to have that avid sample (and any others) that
condition is supposed to check.
So in short, for now I still think this proposed patch is doing the
right thing.

ok then
my reasoning was that
we must support any crafted bitstream and thus turning one marker
into another is always safe.
doing things a bit different has the very small possibility of
introducing an issue ...

[...]