| Message ID | pull.17.ffstaging.FFmpeg.1641530966.ffmpegagent@gmail.com |
|---|---|
| Headers | show |
| Series | avcodec/dvbsubdec, dvdsubdec: don't dump images to disk based on DEBUG define | expand |
On Fri, Jan 7, 2022 at 5:50 AM ffmpegagent <ffmpegagent@gmail.com> wrote: > > It's annoying and unexpected, but still useful at times (as I've realized > just recently). > > This is a follow-up to the earlier submission here: > https://www.mail-archive.com/ffmpeg-devel@ffmpeg.org/msg128080.html > > There has been a comment from Anton, questioning whether the dump-feature is > useful. Meanwhile I came to the conclusion that it can be useful in-fact. It > just shouldn't happen automatically when DEBUG is defined. That's what these > patches do. > > I also added fixes for the fopen() call. > > softworkz (4): > avcodec/dvbsubdec: don't dump images to disk based on DEBUG define > avcodec/dvbsubdec: fix writing ppm > avcodec/dvdsubdec: don't dump images to disk based on DEBUG define > avcodec/dvdsubdec: fix writing ppm > > libavcodec/dvbsubdec.c | 20 +++++++++++++------- > libavcodec/dvdsubdec.c | 11 ++++++++--- > 2 files changed, 21 insertions(+), 10 deletions(-) > > These patches need some squasing. It should've been obvious from them being duplicated. - Hendrik
On Fri, Jan 7, 2022 at 11:20 AM Hendrik Leppkes <h.leppkes@gmail.com> wrote: > > On Fri, Jan 7, 2022 at 5:50 AM ffmpegagent <ffmpegagent@gmail.com> wrote: > > > > It's annoying and unexpected, but still useful at times (as I've realized > > just recently). > > > > This is a follow-up to the earlier submission here: > > https://www.mail-archive.com/ffmpeg-devel@ffmpeg.org/msg128080.html > > > > There has been a comment from Anton, questioning whether the dump-feature is > > useful. Meanwhile I came to the conclusion that it can be useful in-fact. It > > just shouldn't happen automatically when DEBUG is defined. That's what these > > patches do. > > > > I also added fixes for the fopen() call. > > > > softworkz (4): > > avcodec/dvbsubdec: don't dump images to disk based on DEBUG define > > avcodec/dvbsubdec: fix writing ppm > > avcodec/dvdsubdec: don't dump images to disk based on DEBUG define > > avcodec/dvdsubdec: fix writing ppm > > > > libavcodec/dvbsubdec.c | 20 +++++++++++++------- > > libavcodec/dvdsubdec.c | 11 ++++++++--- > > 2 files changed, 21 insertions(+), 10 deletions(-) > > > > > > These patches need some squasing. It should've been obvious from them > being duplicated. > Actually I fell into the dvb dvd trap, but regardless the patches are basically identical, so I would still squash them, personally. - Hendrik
> -----Original Message----- > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Hendrik > Leppkes > Sent: Friday, January 7, 2022 11:32 AM > To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org> > Subject: Re: [FFmpeg-devel] [PATCH 0/4] avcodec/dvbsubdec, dvdsubdec: don't > dump images to disk based on DEBUG define > > On Fri, Jan 7, 2022 at 11:20 AM Hendrik Leppkes <h.leppkes@gmail.com> wrote: > > > > On Fri, Jan 7, 2022 at 5:50 AM ffmpegagent <ffmpegagent@gmail.com> wrote: > > > > > > It's annoying and unexpected, but still useful at times (as I've realized > > > just recently). > > > > > > This is a follow-up to the earlier submission here: > > > https://www.mail-archive.com/ffmpeg-devel@ffmpeg.org/msg128080.html > > > > > > There has been a comment from Anton, questioning whether the dump-feature > is > > > useful. Meanwhile I came to the conclusion that it can be useful in-fact. > It > > > just shouldn't happen automatically when DEBUG is defined. That's what > these > > > patches do. > > > > > > I also added fixes for the fopen() call. > > > > > > softworkz (4): > > > avcodec/dvbsubdec: don't dump images to disk based on DEBUG define > > > avcodec/dvbsubdec: fix writing ppm > > > avcodec/dvdsubdec: don't dump images to disk based on DEBUG define > > > avcodec/dvdsubdec: fix writing ppm > > > > > > libavcodec/dvbsubdec.c | 20 +++++++++++++------- > > > libavcodec/dvdsubdec.c | 11 ++++++++--- > > > 2 files changed, 21 insertions(+), 10 deletions(-) > > > > > > > > > > These patches need some squasing. It should've been obvious from them > > being duplicated. > > > > Actually I fell into the dvb dvd trap, but regardless the patches are > basically identical, so I would still squash them, personally. No problem, but in which way? 1. All 4 commits into a single one 2. 4 >> 2 horizontally (a single one for the debug commits and a single one for the fopen commits) 3. 4 >> 2 vertically (a single commit for dvb and a single one for dvd) Thanks, softworkz
On Fri, 7 Jan 2022, ffmpegagent wrote: > It's annoying and unexpected, but still useful at times (as I've realized > just recently). > > This is a follow-up to the earlier submission here: > https://www.mail-archive.com/ffmpeg-devel@ffmpeg.org/msg128080.html > > There has been a comment from Anton, questioning whether the dump-feature is > useful. Meanwhile I came to the conclusion that it can be useful in-fact. It > just shouldn't happen automatically when DEBUG is defined. That's what these > patches do. Well, I kind of agree with Anton, this a debug feature and it should not be added as an option, but simply should be removed from the codebase. Regards, Marton
> -----Original Message----- > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Marton > Balint > Sent: Friday, January 7, 2022 8:57 PM > To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org> > Subject: Re: [FFmpeg-devel] [PATCH 0/4] avcodec/dvbsubdec, dvdsubdec: don't > dump images to disk based on DEBUG define > > > > On Fri, 7 Jan 2022, ffmpegagent wrote: > > > It's annoying and unexpected, but still useful at times (as I've realized > > just recently). > > > > This is a follow-up to the earlier submission here: > > https://www.mail-archive.com/ffmpeg-devel@ffmpeg.org/msg128080.html > > > > There has been a comment from Anton, questioning whether the dump-feature > is > > useful. Meanwhile I came to the conclusion that it can be useful in-fact. > It > > just shouldn't happen automatically when DEBUG is defined. That's what > these > > patches do. > > Well, I kind of agree with Anton, this a debug feature and it should not > be added as an option, but simply should be removed from the codebase. It's not added as a regular option. It's an option that is only available when you compile with DEBUG defined. Isn't this an acceptable compromise? softworkz
On Fri, 7 Jan 2022, Soft Works wrote: > > >> -----Original Message----- >> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Marton >> Balint >> Sent: Friday, January 7, 2022 8:57 PM >> To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org> >> Subject: Re: [FFmpeg-devel] [PATCH 0/4] avcodec/dvbsubdec, dvdsubdec: don't >> dump images to disk based on DEBUG define >> >> >> >> On Fri, 7 Jan 2022, ffmpegagent wrote: >> >>> It's annoying and unexpected, but still useful at times (as I've realized >>> just recently). >>> >>> This is a follow-up to the earlier submission here: >>> https://www.mail-archive.com/ffmpeg-devel@ffmpeg.org/msg128080.html >>> >>> There has been a comment from Anton, questioning whether the dump-feature >> is >>> useful. Meanwhile I came to the conclusion that it can be useful in-fact. >> It >>> just shouldn't happen automatically when DEBUG is defined. That's what >> these >>> patches do. >> >> Well, I kind of agree with Anton, this a debug feature and it should not >> be added as an option, but simply should be removed from the codebase. > > It's not added as a regular option. It's an option that is only available > when you compile with DEBUG defined. Ah, OK. > > Isn't this an acceptable compromise? Well, I am not a fan of leaving DEBUG chunks in the codebase to be honest. But if somebody applies it, then fine with me. Thanks, Marton
> -----Original Message----- > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Marton > Balint > Sent: Friday, January 7, 2022 10:53 PM > To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org> > Subject: Re: [FFmpeg-devel] [PATCH 0/4] avcodec/dvbsubdec, dvdsubdec: don't > dump images to disk based on DEBUG define > > > > On Fri, 7 Jan 2022, Soft Works wrote: > > > > > > >> -----Original Message----- > >> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Marton > >> Balint > >> Sent: Friday, January 7, 2022 8:57 PM > >> To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org> > >> Subject: Re: [FFmpeg-devel] [PATCH 0/4] avcodec/dvbsubdec, dvdsubdec: > don't > >> dump images to disk based on DEBUG define > >> > >> > >> > >> On Fri, 7 Jan 2022, ffmpegagent wrote: > >> > >>> It's annoying and unexpected, but still useful at times (as I've realized > >>> just recently). > >>> > >>> This is a follow-up to the earlier submission here: > >>> https://www.mail-archive.com/ffmpeg-devel@ffmpeg.org/msg128080.html > >>> > >>> There has been a comment from Anton, questioning whether the dump-feature > >> is > >>> useful. Meanwhile I came to the conclusion that it can be useful in-fact. > >> It > >>> just shouldn't happen automatically when DEBUG is defined. That's what > >> these > >>> patches do. > >> > >> Well, I kind of agree with Anton, this a debug feature and it should not > >> be added as an option, but simply should be removed from the codebase. > > > > It's not added as a regular option. It's an option that is only available > > when you compile with DEBUG defined. > > Ah, OK. > > > > > Isn't this an acceptable compromise? > > Well, I am not a fan of leaving DEBUG chunks in the codebase to be > honest. But if somebody applies it, then fine with me. The code exists in the code base for 6 years.. I was about to agree to the removal, but just few days ago I was glad that I could use it, and it might be useful in the near future in the context of subtitle filtering for troubleshooting. Also there's a bugfix patch where it could be useful: avcodec/dvdsubdec: fix incorrect yellow appearance of dvd subtitles https://github.com/ffstaging/FFmpeg/pull/16 https://master.gitmailbox.com/ffmpegdev/pull.16.ffstaging.FFmpeg.1641262759164.ffmpegagent@gmail.com Eventually, I'll submit a 'dumpgraphicsubs' filter for this purpose, and then that code can be removed anyway, but for the time being, I think this PR still improves the situation (no dump files written to disk unexpectedly), sufficiently enough to merge it even without a full removal. Thanks again, softworkz
On Fri, Jan 7, 2022 at 5:14 PM Soft Works <softworkz@hotmail.com> wrote: > > > > > -----Original Message----- > > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Hendrik > > Leppkes > > Sent: Friday, January 7, 2022 11:32 AM > > To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org> > > Subject: Re: [FFmpeg-devel] [PATCH 0/4] avcodec/dvbsubdec, dvdsubdec: don't > > dump images to disk based on DEBUG define > > > > On Fri, Jan 7, 2022 at 11:20 AM Hendrik Leppkes <h.leppkes@gmail.com> wrote: > > > > > > On Fri, Jan 7, 2022 at 5:50 AM ffmpegagent <ffmpegagent@gmail.com> wrote: > > > > > > > > It's annoying and unexpected, but still useful at times (as I've realized > > > > just recently). > > > > > > > > This is a follow-up to the earlier submission here: > > > > https://www.mail-archive.com/ffmpeg-devel@ffmpeg.org/msg128080.html > > > > > > > > There has been a comment from Anton, questioning whether the dump-feature > > is > > > > useful. Meanwhile I came to the conclusion that it can be useful in-fact. > > It > > > > just shouldn't happen automatically when DEBUG is defined. That's what > > these > > > > patches do. > > > > > > > > I also added fixes for the fopen() call. > > > > > > > > softworkz (4): > > > > avcodec/dvbsubdec: don't dump images to disk based on DEBUG define > > > > avcodec/dvbsubdec: fix writing ppm > > > > avcodec/dvdsubdec: don't dump images to disk based on DEBUG define > > > > avcodec/dvdsubdec: fix writing ppm > > > > > > > > libavcodec/dvbsubdec.c | 20 +++++++++++++------- > > > > libavcodec/dvdsubdec.c | 11 ++++++++--- > > > > 2 files changed, 21 insertions(+), 10 deletions(-) > > > > > > > > > > > > > > These patches need some squasing. It should've been obvious from them > > > being duplicated. > > > > > > > Actually I fell into the dvb dvd trap, but regardless the patches are > > basically identical, so I would still squash them, personally. > > No problem, but in which way? > > 1. All 4 commits into a single one > 2. 4 >> 2 horizontally (a single one for the debug commits and a single > one for the fopen commits) > 3. 4 >> 2 vertically (a single commit for dvb and a single one for dvd) > I figured combining the patches that do the exact same thing for the two different decoders. - Hendrik
> -----Original Message----- > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Hendrik > Leppkes > Sent: Monday, January 10, 2022 11:31 AM > To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org> > Subject: Re: [FFmpeg-devel] [PATCH 0/4] avcodec/dvbsubdec, dvdsubdec: don't > dump images to disk based on DEBUG define > > On Fri, Jan 7, 2022 at 5:14 PM Soft Works <softworkz@hotmail.com> wrote: > > > > > > > > > -----Original Message----- > > > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Hendrik > > > Leppkes > > > Sent: Friday, January 7, 2022 11:32 AM > > > To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org> > > > Subject: Re: [FFmpeg-devel] [PATCH 0/4] avcodec/dvbsubdec, dvdsubdec: > don't > > > dump images to disk based on DEBUG define > > > > > > On Fri, Jan 7, 2022 at 11:20 AM Hendrik Leppkes <h.leppkes@gmail.com> > wrote: > > > > > > > > On Fri, Jan 7, 2022 at 5:50 AM ffmpegagent <ffmpegagent@gmail.com> > wrote: > > > > > > > > > > It's annoying and unexpected, but still useful at times (as I've > realized > > > > > just recently). > > > > > > > > > > This is a follow-up to the earlier submission here: > > > > > https://www.mail-archive.com/ffmpeg-devel@ffmpeg.org/msg128080.html > > > > > > > > > > There has been a comment from Anton, questioning whether the dump- > feature > > > is > > > > > useful. Meanwhile I came to the conclusion that it can be useful in- > fact. > > > It > > > > > just shouldn't happen automatically when DEBUG is defined. That's > what > > > these > > > > > patches do. > > > > > > > > > > I also added fixes for the fopen() call. > > > > > > > > > > softworkz (4): > > > > > avcodec/dvbsubdec: don't dump images to disk based on DEBUG define > > > > > avcodec/dvbsubdec: fix writing ppm > > > > > avcodec/dvdsubdec: don't dump images to disk based on DEBUG define > > > > > avcodec/dvdsubdec: fix writing ppm > > > > > > > > > > libavcodec/dvbsubdec.c | 20 +++++++++++++------- > > > > > libavcodec/dvdsubdec.c | 11 ++++++++--- > > > > > 2 files changed, 21 insertions(+), 10 deletions(-) > > > > > > > > > > > > > > > > > > These patches need some squasing. It should've been obvious from them > > > > being duplicated. > > > > > > > > > > Actually I fell into the dvb dvd trap, but regardless the patches are > > > basically identical, so I would still squash them, personally. > > > > No problem, but in which way? > > > > 1. All 4 commits into a single one > > 2. 4 >> 2 horizontally (a single one for the debug commits and a single > > one for the fopen commits) > > 3. 4 >> 2 vertically (a single commit for dvb and a single one for dvd) > > > > I figured combining the patches that do the exact same thing for the > two different decoders. OK, will do. Thank you, softworkz