[FWD] [PATCH] immediate decoding of dvd stills
Some time ago someone posted this small patch to permit decoding dvd still images without setting LOW_DELAY; unfortunately it seems the patch was forgotten Is it ok to commit? If not, what's the right way to proceed?
Hi On Sat, Nov 25, 2006 at 03:13:43PM +0100, Nico Sabbi wrote:
Some time ago someone posted this small patch to permit decoding dvd still images without setting LOW_DELAY; unfortunately it seems the patch was forgotten
Is it ok to commit? If not, what's the right way to proceed?
Index: libavcodec/mpeg12.c =================================================================== RCS file: /cvsroot/ffmpeg/ffmpeg/libavcodec/mpeg12.c,v retrieving revision 1.248 diff -u -r1.248 mpeg12.c --- libavcodec/mpeg12.c 4 Feb 2006 20:32:02 -0000 1.248 +++ libavcodec/mpeg12.c 24 Feb 2006 13:15:29 -0000 @@ -2983,6 +2983,17 @@ } }
+ /* look for SEQ_END_CODE at the last data in this buffer*/ + /* dvd's won't send the next frame start on still images*/ + /* state should hold the last startcode if one was found above*/ + /* i will point to the position after that startcode */ + if(!pc->frame_start_found){
this could be a else after the if(pc->frame_start_found) below
+ if(state == SEQ_END_CODE){
+ pc->state=-1;
i this needed?
+ return i; + } + }
indention is not 4 spaces
+ if(pc->frame_start_found){ /* EOF considered as end of frame */ if (buf_size == 0)
except these its ok if it works and doesnt breal anything [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB In the past you could go to a library and read, borrow or copy any book Today you'd get arrested for mere telling someone where the library is
Michael Niedermayer wrote:
Hi
+ if(pc->frame_start_found){ /* EOF considered as end of frame */ if (buf_size == 0)
except these its ok if it works and doesnt breal anything
[...]
uhm done, but it's still not enough to force lavc to release immediately Iframes followed by 0x1b7 code (low_delay is still needed). I must have used wrong samples. Any clue?
Hi On Mon, Nov 27, 2006 at 11:17:12PM +0100, Nico Sabbi wrote:
Michael Niedermayer wrote:
Hi
+ if(pc->frame_start_found){ /* EOF considered as end of frame */ if (buf_size == 0)
except these its ok if it works and doesnt breal anything
[...]
uhm done, but it's still not enough to force lavc to release immediately Iframes followed by 0x1b7 code (low_delay is still needed). I must have used wrong samples. Any clue?
yes, after thinking about this for a second ... the 0x1b7 should be sent to the decoder as its own packet and the decoder should if needed then output the last frame (as if it has been feeded with a 0 sized packet at the end) something like: - if (buf_size == 0) { + if (buf_size == 0 || !memcmp(buf, (char[]){0,0,1,BF}, 4) { /* special case for last picture */ if (s2->low_delay==0 && s2->next_picture_ptr) { [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB In the past you could go to a library and read, borrow or copy any book Today you'd get arrested for mere telling someone where the library is
Michael Niedermayer wrote:
something like:
- if (buf_size == 0) { + if (buf_size == 0 || !memcmp(buf, (char[]){0,0,1,BF}, 4) { /* special case for last picture */ if (s2->low_delay==0 && s2->next_picture_ptr) {
[...]
updated
Nico Sabbi wrote:
Michael Niedermayer wrote:
something like:
- if (buf_size == 0) { + if (buf_size == 0 || !memcmp(buf, (char[]){0,0,1,BF}, 4) { /* special case for last picture */ if (s2->low_delay==0 && s2->next_picture_ptr) {
[...]
updated
- if (buf_size == 0) { + if(!memcmp(buf, (char[]){0,0,1,0xB7}, 4)) + endsequence = 1;
idiot me; patch fixed. No regression reported by make test
Hi On Tue, Nov 28, 2006 at 11:30:54PM +0100, Nico Sabbi wrote:
Nico Sabbi wrote:
Michael Niedermayer wrote:
something like:
- if (buf_size == 0) { + if (buf_size == 0 || !memcmp(buf, (char[]){0,0,1,BF}, 4) { /* special case for last picture */ if (s2->low_delay==0 && s2->next_picture_ptr) {
[...]
updated
- if (buf_size == 0) { + if(!memcmp(buf, (char[]){0,0,1,0xB7}, 4)) + endsequence = 1;
idiot me; patch fixed. No regression reported by make test
Index: libavcodec/mpeg12.c =================================================================== --- libavcodec/mpeg12.c (revisione 7167) +++ libavcodec/mpeg12.c (copia locale) @@ -3037,6 +3037,12 @@ } } } + }else{ + /* look for SEQ_END_CODE at the last data in this buffer*/ + /* dvd's won't send the next frame start on still images*/ + /* SEQ_END_CODE will have to stay at the beginning of a frame*/ + if(state == SEQ_END_CODE) + return i-3; } pc->state= state; return END_NOT_FOUND; @@ -3054,9 +3060,11 @@ int ret, input_size; AVFrame *picture = data; MpegEncContext *s2 = &s->mpeg_enc_ctx; + int endsequence = 0; dprintf("fill_buffer\n");
- if (buf_size == 0) { + endsequence = (buf_size > 3 && !memcmp(buf, (char[]){0,0,1,0xB7}, 4)); + if (buf_size == 0 || endsequence) { /* special case for last picture */ if (s2->low_delay==0 && s2->next_picture_ptr) { *picture= *(AVFrame*)s2->next_picture_ptr; @@ -3064,7 +3072,7 @@
*data_size = sizeof(AVFrame); } - return 0; + return endsequence ? 4 : 0;
is there a problem with return buf_size; ? if not then if (buf_size == 0 || !memcmp(buf, (char[]){0,0,1,0xB7}, 4)) can be used and the code is a little simpler (note if buf_size>0) then there always need to be at least FF_INPUT_BUFFER_PADDING_SIZE bytes allocated so no need to check for >= 4 except that patch ok if it works [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB In the past you could go to a library and read, borrow or copy any book Today you'd get arrested for mere telling someone where the library is
Michael Niedermayer wrote:
is there a problem with return buf_size; ?
no, but why returning ret < bytes actually consumed? I find it misleadig
if not then if (buf_size == 0 || !memcmp(buf, (char[]){0,0,1,0xB7}, 4)) can be used and the code is a little simpler (note if buf_size>0) then there always need to be at least FF_INPUT_BUFFER_PADDING_SIZE bytes allocated so no need to check for >= 4
ok
except that patch ok if it works
[...]
uhm, it works only this way. Please, check it.
Hi On Wed, Nov 29, 2006 at 01:01:58AM +0100, Nico Sabbi wrote:
Michael Niedermayer wrote:
is there a problem with return buf_size; ?
no, but why returning ret < bytes actually consumed? I find it misleadig
hmm, i didnt suggest return 0, but i see the problem now, i assumed that there would always be 1 frame per packet and the end code also alone in one that of course isnt guranteed even though it should always be the case with an AVParser [...]
Index: libavcodec/mpeg12.c =================================================================== --- libavcodec/mpeg12.c (revisione 7167) +++ libavcodec/mpeg12.c (copia locale) @@ -3037,6 +3037,12 @@ } } } + }else{ + /* look for SEQ_END_CODE at the last data in this buffer*/ + /* dvd's won't send the next frame start on still images*/ + /* SEQ_END_CODE will have to stay at the beginning of a frame*/ + if(state == SEQ_END_CODE) + return i-3; } pc->state= state; return END_NOT_FOUND; @@ -3054,11 +3060,12 @@ int ret, input_size; AVFrame *picture = data; MpegEncContext *s2 = &s->mpeg_enc_ctx; + int endsequence = !memcmp(buf, (char[]){0,0,1,0xB7}, 4); dprintf("fill_buffer\n");
buf can be NULL if buf_size==0
- if (buf_size == 0) { + if (buf_size == 0 || endsequence) { /* special case for last picture */ - if (s2->low_delay==0 && s2->next_picture_ptr) { + if (s2->low_delay==0 && !endsequence && s2->next_picture_ptr) { *picture= *(AVFrame*)s2->next_picture_ptr; s2->next_picture_ptr= NULL;
this is wrong, if you output a picture, then it must be removed otherwise it would be output again with the next I frame and no iam not saying s2->next_picture_ptr= NULL is enough for this, just that doing nothing is definitly not correct [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB In the past you could go to a library and read, borrow or copy any book Today you'd get arrested for mere telling someone where the library is
Michael Niedermayer wrote:
this is wrong, if you output a picture, then it must be removed otherwise it would be output again with the next I frame
and no iam not saying s2->next_picture_ptr= NULL is enough for this, just that doing nothing is definitly not correct
[...]
this patch works exactly as I need (mplayer -fps 1 file.m2v shows the stills with 0 delay without setting low_delay in the flags), but I don't know if it's even remotely acceptable or how much it sucks, apologies in advance :) The seq_end_code is stored at the end of the frame, and based on its presence slice_end() works as if low_delay was set; ff_draw_horiz_band() works on ->current_picture_ptr if the codec is mpeg12 && pict_type== I_frame and ->temp_ref==0 (that should be true for all mpeg12 stills). If this method is wrong I'd like to have some direction, please :) At the moment I'm just stabbing in the dark
Hi On Fri, Dec 01, 2006 at 01:24:13AM +0100, Nico Sabbi wrote:
Michael Niedermayer wrote:
this is wrong, if you output a picture, then it must be removed otherwise it would be output again with the next I frame
and no iam not saying s2->next_picture_ptr= NULL is enough for this, just that doing nothing is definitly not correct
[...]
this patch works exactly as I need (mplayer -fps 1 file.m2v shows the stills with 0 delay without setting low_delay in the flags), but I don't know if it's even remotely acceptable or how much it sucks, apologies in advance :)
The seq_end_code is stored at the end of the frame, and based on its presence slice_end() works as if low_delay was set; ff_draw_horiz_band() works on ->current_picture_ptr if the codec is mpeg12 && pict_type== I_frame and ->temp_ref==0
this is wrong, it totally violate the spec and it will cause artifacts for every i frame with temp_ref==0
(that should be true for all mpeg12 stills). If this method is wrong I'd like to have some direction, please :) At the moment I'm just stabbing in the dark
ok, see below, i hope it makes sense as iam tired if not ill retry tomorow (S=still image, E=seq end, please correct me if some assumtion is wrong) as stored: I0 P3 B1 B2 P6 B4 B5 P7 S8 E9 IA PD BB BC decoding with your code: I0 B1 B2 P3 B4 B5 P6 S8 S8?IA BB BC what should be output: I0 B1 B2 P3 B4 B5 P6 P7 S8 IA BB BC your code looses the last frame before the still image (iam assuming that this shouldnt happen, but i dunno, maybe iam wrong in what spec is the still picture stuff?) [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB In the past you could go to a library and read, borrow or copy any book Today you'd get arrested for mere telling someone where the library is
Michael Niedermayer wrote:
ok, see below, i hope it makes sense as iam tired if not ill retry tomorow
(S=still image, E=seq end, please correct me if some assumtion is wrong)
as stored: I0 P3 B1 B2 P6 B4 B5 P7 S8 E9 IA PD BB BC decoding with your code: I0 B1 B2 P3 B4 B5 P6 S8 S8?IA BB BC what should be output: I0 B1 B2 P3 B4 B5 P6 P7 S8 IA BB BC
your code looses the last frame before the still image (iam assuming that this shouldnt happen, but i dunno, maybe iam wrong in what spec is the still picture stuff?)
[...]
I didn't find a formal definition of still image in the specs (there's only written that sequence_end_code terminates a sequence), but they are generally coded as: sequence header+all sequence_extension headers I frame with temp_ref=0 sequence_end so in your example S8 would be S0 00000000: sequence hdr: 720x576, a/f:23, bitrate=22500 0000004c: sequence extension hdr, frame rate extension: 1/1 00000056: sequence display extension hdr 00000062: GOP: 0:00:00.00, closed 0000006a: picture hdr, frametype=I, temporal=0 00000072: picture coding extension hdr, top trf=2 0000cc29: sequence end code 0000cc2d: sequence hdr: 720x576, a/f:23, bitrate=22500 0000cc79: sequence extension hdr, frame rate extension: 1/1 0000cc83: sequence display extension hdr 0000cc8f: GOP: 0:00:00.00, closed 0000cc97: picture hdr, frametype=I, temporal=0 0000cc9f: picture coding extension hdr, top trf=2 00017d82: sequence end code 00017d86: sequence hdr: 720x576, a/f:23, bitrate=22500 00017dd2: sequence extension hdr, frame rate extension: 1/1 00017ddc: sequence display extension hdr 00017de8: GOP: 0:00:00.00, closed 00017df0: picture hdr, frametype=I, temporal=0 00017df8: picture coding extension hdr, top trf=2 000242d1: sequence end code
Hi On Fri, Dec 01, 2006 at 11:10:03PM +0100, Nico Sabbi wrote:
Michael Niedermayer wrote:
ok, see below, i hope it makes sense as iam tired if not ill retry tomorow
(S=still image, E=seq end, please correct me if some assumtion is wrong)
as stored: I0 P3 B1 B2 P6 B4 B5 P7 S8 E9 IA PD BB BC decoding with your code: I0 B1 B2 P3 B4 B5 P6 S8 S8?IA BB BC what should be output: I0 B1 B2 P3 B4 B5 P6 P7 S8 IA BB BC
your code looses the last frame before the still image (iam assuming that this shouldnt happen, but i dunno, maybe iam wrong in what spec is the still picture stuff?)
[...]
I didn't find a formal definition of still image in the specs (there's only written that sequence_end_code terminates a sequence), but they are generally coded as:
sequence header+all sequence_extension headers I frame with temp_ref=0 sequence_end
low_delay=0 for them or? if so then forcing low_delay=1 is not correct [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB In the past you could go to a library and read, borrow or copy any book Today you'd get arrested for mere telling someone where the library is
Michael Niedermayer wrote:
Hi
On Fri, Dec 01, 2006 at 11:10:03PM +0100, Nico Sabbi wrote:
sequence header+all sequence_extension headers I frame with temp_ref=0 sequence_end
low_delay=0 for them or? if so then forcing low_delay=1 is not correct
[...]
sorry, I don't understand what you mean in the last sentence. Is this patch acceptable? If the frame ends with a sequence end code && pict_type==I && temp_ref==0 then temporarily set low_delay=1. Before returning restore the original low_delay. Together with the second patch it works correctly with mplayer.
Nico Sabbi wrote:
Is this patch acceptable? If the frame ends with a sequence end code && pict_type==I && temp_ref==0 then temporarily set low_delay=1. Before returning restore the original low_delay. Together with the second patch it works correctly with mplayer.
------------------------------------------------------------------------
@@ -3127,11 +3131,16 @@ mpeg1_decode_sequence(avctx, buf_ptr, input_size); break; + case SEQ_END_CODE: + seqend=1; + break;
oops, this block isn't necessary. patch updated
Hi, Bump on OLD OLD issue. Still no knews on this is there? I would love to replace libmpeg2 with ffmpeg for dvd video decoding, but this is still a blocker. At some point since back then, the parser for mpeg video has been changed to output the sequence end byte as part of the video packet so a patch like this is also required: diff --git a/libavcodec/mpeg12.c b/libavcodec/mpeg12.c index bd858a5..3fd68e3 100644 --- a/libavcodec/mpeg12.c +++ b/libavcodec/mpeg12.c @@ -2280,7 +2280,7 @@ static int mpeg_decode_frame(AVCodecContext *avctx, MpegEncContext *s2 = &s->mpeg_enc_ctx; av_dlog(avctx, "fill_buffer\n"); - if (buf_size == 0 || (buf_size == 4 && AV_RB32(buf) == SEQ_END_CODE)) { + if (buf_size == 0 || (buf_size >= 4 && AV_RB32(buf+buf_size-4) == SEQ_END_CODE)) { /* special case for last picture */ if (s2->low_delay==0 && s2->next_picture_ptr) { *picture= *(AVFrame*)s2->next_picture_ptr; Would something like a bug workaround flag be acceptable to get this given that it's fully know it's a dvd with still frames on codec open? /Joakim
On Tue, Jun 21, 2011 at 07:18:22PM +0000, Joakim Plate wrote:
Hi,
Bump on OLD OLD issue. Still no knews on this is there? I would love to replace libmpeg2 with ffmpeg for dvd video decoding, but this is still a blocker.
At some point since back then, the parser for mpeg video has been changed to output the sequence end byte as part of the video packet
do you know which commit changed that?
so a patch like this is also required:
this doesnt look correct, i suspect it will loose a picture
diff --git a/libavcodec/mpeg12.c b/libavcodec/mpeg12.c index bd858a5..3fd68e3 100644 --- a/libavcodec/mpeg12.c +++ b/libavcodec/mpeg12.c @@ -2280,7 +2280,7 @@ static int mpeg_decode_frame(AVCodecContext *avctx, MpegEncContext *s2 = &s->mpeg_enc_ctx; av_dlog(avctx, "fill_buffer\n");
- if (buf_size == 0 || (buf_size == 4 && AV_RB32(buf) == SEQ_END_CODE)) { + if (buf_size == 0 || (buf_size >= 4 && AV_RB32(buf+buf_size-4) == SEQ_END_CODE)) { /* special case for last picture */ if (s2->low_delay==0 && s2->next_picture_ptr) { *picture= *(AVFrame*)s2->next_picture_ptr;
Would something like a bug workaround flag be acceptable to get this given that it's fully know it's a dvd with still frames on codec open?
id like to understand better why we have a regression here. If i understand you correctly you say this worked before [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB When the tyrant has disposed of foreign enemies by conquest or treaty, and there is nothing more to fear from them, then he is always stirring up some war or other, in order that the people may require a leader. -- Plato
Hi On Sat, Dec 02, 2006 at 03:29:23PM +0100, Nico Sabbi wrote:
Michael Niedermayer wrote:
Hi
On Fri, Dec 01, 2006 at 11:10:03PM +0100, Nico Sabbi wrote:
sequence header+all sequence_extension headers I frame with temp_ref=0 sequence_end
low_delay=0 for them or? if so then forcing low_delay=1 is not correct
[...]
sorry, I don't understand what you mean in the last sentence.
a mpeg decoder MUST conform to the mpeg specification the mpeg specificaton leaves no doubt that low_delay=0 in the header means low_delay=0
Is this patch acceptable? If the frame ends with a sequence end code && pict_type==I && temp_ref==0 then temporarily set low_delay=1. Before returning restore the original low_delay. Together with the second patch it works correctly with mplayer.
no, it looses the last frame and simply isnt correct, if they would have wanted the decoder to behave as low_delay=1 they would have set that in the header for still images, so i do not think this is the correct way to handle it (feel free to point me to some spec which says otherwise) also think of concatenated streams this should make the issue more obvious I0 P1 P2 E3 <a few seconds of zero padding>I4 P5 P6 - I0 P1 P2 I4 P5 the issue here is the last frame of the previous sequence is shown when the next sequence starts not when the sequence ended - I0 P1 P2 - I4 P5 so the correct solution seems to be: all normal frames get decoded and displayed as they are currently if an sequence end is hit, the last frame is output (and removed so it wont be output twice) [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB In the past you could go to a library and read, borrow or copy any book Today you'd get arrested for mere telling someone where the library is
participants (5)
-
Joakim Plate -
Michael Niedermayer -
michaelni＠gmx.at
-
nicola_sabbi＠fastwebnet.it -
nsabbi＠email.it