Re: [FFmpeg-devel] [PATCH 1/2] Revert "avcodec/qtrle: Do not output duplicated frames on insufficient input"
On Tue, May 14, 2019 at 10:31:00PM +0200, Marton Balint wrote: > > > On Wed, 8 May 2019, Michael Niedermayer wrote: > > >On Tue, May 07, 2019 at 02:03:22AM +0200, Marton Balint wrote: > >> > >> > >>On Tue, 7 May 2019, Michael Niedermayer wrote: > >> > >>>On Sun, May 05, 2019 at 08:51:08PM +0200, Marton Balint wrote: > This reverts commit a9dacdeea6168787a142209bd19fdd74aefc9dd6. > > I don't think it is a good idea to drop frames from CFR input just > because they > are duplicated, that can cause issues for API users expecting CFR input. > Also > it can cause issues at the end of file, if the last frame is a duplicated > frame. > > Fixes ticket #7880. > > Signed-off-by: Marton Balint > --- > libavcodec/qtrle.c| 12 ++--- > tests/ref/fate/qtrle-8bit | 109 > ++ > 2 files changed, 115 insertions(+), 6 deletions(-) > >>> > >>>This change would make the decoder quite a bit slower. > >> > >>I guess that can be easily fixed by only copying the buffer if it really is > >>a different frame. > >> > >>>It also would make encoding the output harder. > >>>For example motion estimation would be run over unchanged frames even when > >>>no cfr is wanted. > >> > >>The performance penalty is much more acceptable to me than the issue > >>described in the ticket. Do you see a straightforward way to fix it other > >>than reverting? > > > >decoders can in general have frames at the end which need to be flushed > >out. For example IPB mpeg1/2/4/... > >In the same way the decoder could output a last frame representing the end > >exactly > > Yeah, that probably would fix the ticket. However I am still worried that it > is just a matter of time before somebody reports something else that is > broken because of VFR and unknown frame duration. > > Do you plan on working on fixing the ticket? It is a regression, so > prefeably it should either be fixed in a reasonable time frame or if nobody > is willing to do it then we should revert. ill post a patch that implements the last frame flush idea. thx [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB The greatest way to live with honor in this world is to be what we pretend to be. -- Socrates signature.asc Description: PGP signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 1/2] Revert "avcodec/qtrle: Do not output duplicated frames on insufficient input"
On Wed, 8 May 2019, Michael Niedermayer wrote: On Tue, May 07, 2019 at 02:03:22AM +0200, Marton Balint wrote: On Tue, 7 May 2019, Michael Niedermayer wrote: On Sun, May 05, 2019 at 08:51:08PM +0200, Marton Balint wrote: This reverts commit a9dacdeea6168787a142209bd19fdd74aefc9dd6. I don't think it is a good idea to drop frames from CFR input just because they are duplicated, that can cause issues for API users expecting CFR input. Also it can cause issues at the end of file, if the last frame is a duplicated frame. Fixes ticket #7880. Signed-off-by: Marton Balint --- libavcodec/qtrle.c| 12 ++--- tests/ref/fate/qtrle-8bit | 109 ++ 2 files changed, 115 insertions(+), 6 deletions(-) This change would make the decoder quite a bit slower. I guess that can be easily fixed by only copying the buffer if it really is a different frame. It also would make encoding the output harder. For example motion estimation would be run over unchanged frames even when no cfr is wanted. The performance penalty is much more acceptable to me than the issue described in the ticket. Do you see a straightforward way to fix it other than reverting? decoders can in general have frames at the end which need to be flushed out. For example IPB mpeg1/2/4/... In the same way the decoder could output a last frame representing the end exactly Yeah, that probably would fix the ticket. However I am still worried that it is just a matter of time before somebody reports something else that is broken because of VFR and unknown frame duration. Do you plan on working on fixing the ticket? It is a regression, so prefeably it should either be fixed in a reasonable time frame or if nobody is willing to do it then we should revert. The original ossfuzz issue might also be fixed by keeping only the hunk which moves ff_reget_buffer down. Regards, Marton ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 1/2] Revert "avcodec/qtrle: Do not output duplicated frames on insufficient input"
> > if you dont return 3 fields you break the normative specification. This > speaks > about the "output of the decoding process" not how to interpret the output. > > I bring MPEG2 up here because we dont do what the normative spec says > because it doesnt make sense for us. It does make sense if you output on a > analoge interlaced PAL/NTSC screen. It is fundamentally the same as the > CFR case, on one side a interlaced display as factor on the other a output > only capable to handle fixed duration (CFR) frames. > > About CFR codecs, the cases this is about are generally input packets that > code the equivalent of "nothing changed". In frameworks that allow only CFR > an implementation would produce a duplicated frame. In frameworks that > allow > VFR an implementation can omit the duplicated frame. > I don't really care about qtrle here but this is not comparable. If you are a cfr output device, you *know* that you have to duplicate a field because of the flag that's exported and this duration is clearly defined. But with the qtrle patch, you don't know how long the duration of the frame just returned is. Kieran ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 1/2] Revert "avcodec/qtrle: Do not output duplicated frames on insufficient input"
On Tue, May 07, 2019 at 01:39:44AM +0200, Hendrik Leppkes wrote: > On Tue, May 7, 2019 at 12:34 AM Michael Niedermayer > wrote: > > > > On Sun, May 05, 2019 at 08:51:08PM +0200, Marton Balint wrote: > > > This reverts commit a9dacdeea6168787a142209bd19fdd74aefc9dd6. > > > > > > I don't think it is a good idea to drop frames from CFR input just > > > because they > > > are duplicated, that can cause issues for API users expecting CFR input. > > > Also > > > it can cause issues at the end of file, if the last frame is a duplicated > > > frame. > > > > > > Fixes ticket #7880. > > > > > > Signed-off-by: Marton Balint > > > --- > > > libavcodec/qtrle.c| 12 ++--- > > > tests/ref/fate/qtrle-8bit | 109 > > > ++ > > > 2 files changed, 115 insertions(+), 6 deletions(-) > > > > This change would make the decoder quite a bit slower. It also would make > > encoding the output harder. > > For example motion estimation would be run over unchanged frames even when > > no cfr is wanted. > > This is simple: > There is X input packets, any other decoder outputs X output frames. > FFmpeg outputs Y output frames (where Y < X). How can this be correct > decoding? > > If you want to lesten the burden of static frames, a filter to detect > duplicates and make a stream VFR is what you should suggest, not > making decoders act "unexpectedly". > > > > > Also if one for consistency wants every decoder to not drop duplicated > > things > > that will cause some major problems in other decoders. > > Iam thinking of MPEG2 here, where the duplication is at a field level > > perfectly progressive material would be turned into some mess with field > > repetition in that case. Again undoing that in a subsequent stage would be > > quite a bit harder and wastefull > > > > There is quite a fundamental difference between CFR codecs where we > end up not generating output for an input packet just because we feel > like it, and the thought of somehow interpreting field repeat > metadata. That just smells like deflection, lets not go there. in ISO/IEC 13818-2: 1995 (E), repeat_first_field is part of the picture coding extension, if its removed it will no longer decode. Iam not sure calling this metadata is accurate. also the description of the field is: [...]If progressive_sequence is equal to 0 and progressive_frame is equal to 1: If this flag is set to 0, the output of the decoding process corresponding to this reconstructed frame consists of two fields. The first field (top or bottom field as identified by top_field_first) is followed by the other field. If it is set to 1, the output of the decoding process corresponding to this reconstructed frame consists of three fields. The first field (top or bottom field as identified by top_field_first) is followed by the other field, then the first field is repeated. [...] if you dont return 3 fields you break the normative specification. This speaks about the "output of the decoding process" not how to interpret the output. I bring MPEG2 up here because we dont do what the normative spec says because it doesnt make sense for us. It does make sense if you output on a analoge interlaced PAL/NTSC screen. It is fundamentally the same as the CFR case, on one side a interlaced display as factor on the other a output only capable to handle fixed duration (CFR) frames. About CFR codecs, the cases this is about are generally input packets that code the equivalent of "nothing changed". In frameworks that allow only CFR an implementation would produce a duplicated frame. In frameworks that allow VFR an implementation can omit the duplicated frame. [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Avoid a single point of failure, be that a person or equipment. signature.asc Description: PGP signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 1/2] Revert "avcodec/qtrle: Do not output duplicated frames on insufficient input"
On Tue, May 07, 2019 at 02:03:22AM +0200, Marton Balint wrote: > > > On Tue, 7 May 2019, Michael Niedermayer wrote: > > >On Sun, May 05, 2019 at 08:51:08PM +0200, Marton Balint wrote: > >>This reverts commit a9dacdeea6168787a142209bd19fdd74aefc9dd6. > >> > >>I don't think it is a good idea to drop frames from CFR input just because > >>they > >>are duplicated, that can cause issues for API users expecting CFR input. > >>Also > >>it can cause issues at the end of file, if the last frame is a duplicated > >>frame. > >> > >>Fixes ticket #7880. > >> > >>Signed-off-by: Marton Balint > >>--- > >> libavcodec/qtrle.c| 12 ++--- > >> tests/ref/fate/qtrle-8bit | 109 > >> ++ > >> 2 files changed, 115 insertions(+), 6 deletions(-) > > > >This change would make the decoder quite a bit slower. > > I guess that can be easily fixed by only copying the buffer if it really is > a different frame. > > >It also would make encoding the output harder. > >For example motion estimation would be run over unchanged frames even when > >no cfr is wanted. > > The performance penalty is much more acceptable to me than the issue > described in the ticket. Do you see a straightforward way to fix it other > than reverting? decoders can in general have frames at the end which need to be flushed out. For example IPB mpeg1/2/4/... In the same way the decoder could output a last frame representing the end exactly [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB The educated differ from the uneducated as much as the living from the dead. -- Aristotle signature.asc Description: PGP signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 1/2] Revert "avcodec/qtrle: Do not output duplicated frames on insufficient input"
On Tue, 7 May 2019, Michael Niedermayer wrote: On Sun, May 05, 2019 at 08:51:08PM +0200, Marton Balint wrote: This reverts commit a9dacdeea6168787a142209bd19fdd74aefc9dd6. I don't think it is a good idea to drop frames from CFR input just because they are duplicated, that can cause issues for API users expecting CFR input. Also it can cause issues at the end of file, if the last frame is a duplicated frame. Fixes ticket #7880. Signed-off-by: Marton Balint --- libavcodec/qtrle.c| 12 ++--- tests/ref/fate/qtrle-8bit | 109 ++ 2 files changed, 115 insertions(+), 6 deletions(-) This change would make the decoder quite a bit slower. I guess that can be easily fixed by only copying the buffer if it really is a different frame. It also would make encoding the output harder. For example motion estimation would be run over unchanged frames even when no cfr is wanted. The performance penalty is much more acceptable to me than the issue described in the ticket. Do you see a straightforward way to fix it other than reverting? Also if one for consistency wants every decoder to not drop duplicated things that will cause some major problems in other decoders. Iam thinking of MPEG2 here, where the duplication is at a field level perfectly progressive material would be turned into some mess with field repetition in that case. Again undoing that in a subsequent stage would be quite a bit harder and wastefull so my oppinion is that its better to produce duplicated things only when needed and not always and hardcoded in the decoder. The problem is that you are creating VFR from CFR behind the scenes even for files which are not meant to be VFR, so people don't expect to get VFR from them. There can be use cases where VFR generation is really beneficial, but it should not be the default IMHO. VFR is always more problematic, yes we mostly handle it, but not without issues here and there. Regards, Marton ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 1/2] Revert "avcodec/qtrle: Do not output duplicated frames on insufficient input"
On Tue, May 7, 2019 at 1:39 AM Hendrik Leppkes wrote: > > On Tue, May 7, 2019 at 12:34 AM Michael Niedermayer > wrote: > > > > On Sun, May 05, 2019 at 08:51:08PM +0200, Marton Balint wrote: > > > This reverts commit a9dacdeea6168787a142209bd19fdd74aefc9dd6. > > > > > > I don't think it is a good idea to drop frames from CFR input just > > > because they > > > are duplicated, that can cause issues for API users expecting CFR input. > > > Also > > > it can cause issues at the end of file, if the last frame is a duplicated > > > frame. > > > > > > Fixes ticket #7880. > > > > > > Signed-off-by: Marton Balint > > > --- > > > libavcodec/qtrle.c| 12 ++--- > > > tests/ref/fate/qtrle-8bit | 109 > > > ++ > > > 2 files changed, 115 insertions(+), 6 deletions(-) > > > > This change would make the decoder quite a bit slower. It also would make > > encoding the output harder. > > For example motion estimation would be run over unchanged frames even when > > no cfr is wanted. > > This is simple: > There is X input packets, any other decoder outputs X output frames. > FFmpeg outputs Y output frames (where Y < X). How can this be correct > decoding? > > If you want to lesten the burden of static frames, a filter to detect > duplicates and make a stream VFR is what you should suggest, not > making decoders act "unexpectedly". > > > > > Also if one for consistency wants every decoder to not drop duplicated > > things > > that will cause some major problems in other decoders. > > Iam thinking of MPEG2 here, where the duplication is at a field level > > perfectly progressive material would be turned into some mess with field > > repetition in that case. Again undoing that in a subsequent stage would be > > quite a bit harder and wastefull > > > > There is quite a fundamental difference between CFR codecs where we > end up not generating output for an input packet just because we feel > like it, and the thought of somehow interpreting field repeat > metadata. That just smells like deflection, lets not go there. > Also since you talk about making a "mess", this class of changes makes quite a mess out of perfectly CFR material, to use your own formulation. :) - Hendrik ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 1/2] Revert "avcodec/qtrle: Do not output duplicated frames on insufficient input"
On Tue, May 7, 2019 at 12:34 AM Michael Niedermayer wrote: > > On Sun, May 05, 2019 at 08:51:08PM +0200, Marton Balint wrote: > > This reverts commit a9dacdeea6168787a142209bd19fdd74aefc9dd6. > > > > I don't think it is a good idea to drop frames from CFR input just because > > they > > are duplicated, that can cause issues for API users expecting CFR input. > > Also > > it can cause issues at the end of file, if the last frame is a duplicated > > frame. > > > > Fixes ticket #7880. > > > > Signed-off-by: Marton Balint > > --- > > libavcodec/qtrle.c| 12 ++--- > > tests/ref/fate/qtrle-8bit | 109 > > ++ > > 2 files changed, 115 insertions(+), 6 deletions(-) > > This change would make the decoder quite a bit slower. It also would make > encoding the output harder. > For example motion estimation would be run over unchanged frames even when > no cfr is wanted. This is simple: There is X input packets, any other decoder outputs X output frames. FFmpeg outputs Y output frames (where Y < X). How can this be correct decoding? If you want to lesten the burden of static frames, a filter to detect duplicates and make a stream VFR is what you should suggest, not making decoders act "unexpectedly". > > Also if one for consistency wants every decoder to not drop duplicated things > that will cause some major problems in other decoders. > Iam thinking of MPEG2 here, where the duplication is at a field level > perfectly progressive material would be turned into some mess with field > repetition in that case. Again undoing that in a subsequent stage would be > quite a bit harder and wastefull > There is quite a fundamental difference between CFR codecs where we end up not generating output for an input packet just because we feel like it, and the thought of somehow interpreting field repeat metadata. That just smells like deflection, lets not go there. - Hendrik ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 1/2] Revert "avcodec/qtrle: Do not output duplicated frames on insufficient input"
On Sun, May 05, 2019 at 08:51:08PM +0200, Marton Balint wrote: > This reverts commit a9dacdeea6168787a142209bd19fdd74aefc9dd6. > > I don't think it is a good idea to drop frames from CFR input just because > they > are duplicated, that can cause issues for API users expecting CFR input. Also > it can cause issues at the end of file, if the last frame is a duplicated > frame. > > Fixes ticket #7880. > > Signed-off-by: Marton Balint > --- > libavcodec/qtrle.c| 12 ++--- > tests/ref/fate/qtrle-8bit | 109 > ++ > 2 files changed, 115 insertions(+), 6 deletions(-) This change would make the decoder quite a bit slower. It also would make encoding the output harder. For example motion estimation would be run over unchanged frames even when no cfr is wanted. Also if one for consistency wants every decoder to not drop duplicated things that will cause some major problems in other decoders. Iam thinking of MPEG2 here, where the duplication is at a field level perfectly progressive material would be turned into some mess with field repetition in that case. Again undoing that in a subsequent stage would be quite a bit harder and wastefull so my oppinion is that its better to produce duplicated things only when needed and not always and hardcoded in the decoder. Thanks [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB It is what and why we do it that matters, not just one of them. signature.asc Description: PGP signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 1/2] Revert "avcodec/qtrle: Do not output duplicated frames on insufficient input"
On Sun, May 5, 2019 at 8:51 PM Marton Balint wrote: > > This reverts commit a9dacdeea6168787a142209bd19fdd74aefc9dd6. > > I don't think it is a good idea to drop frames from CFR input just because > they > are duplicated, that can cause issues for API users expecting CFR input. Also > it can cause issues at the end of file, if the last frame is a duplicated > frame. > > Fixes ticket #7880. > qtrle is not the only codec such a change was applied to, and I've in the past objected to this class of changes without much support. "Security" arguments were used to squat any counter-argument. - Hendrik ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 1/2] Revert "avcodec/qtrle: Do not output duplicated frames on insufficient input"
Am So., 5. Mai 2019 um 20:51 Uhr schrieb Marton Balint : > > This reverts commit a9dacdeea6168787a142209bd19fdd74aefc9dd6. > > I don't think it is a good idea to drop frames from CFR input just because > they > are duplicated, that can cause issues for API users expecting CFR input. Also > it can cause issues at the end of file, if the last frame is a duplicated > frame. > Fixes ticket #7880. I am sure that this issue is reproducible with other codecs so I would prefer a solution in ffmpeg. Carl Eugen ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
