| Message ID | 9017e9a3-8701-e787-7ec8-b576ed7accab@mark-plomer.de |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] avcodec/dv_profile: dv files with wrong dsf flag - detect via buf_size | expand |
| Context | Check | Description |
|---|---|---|
| andriy/x86_make | success | Make finished |
| andriy/x86_make_fate | success | Make fate finished |
| andriy/PPC64_make | success | Make finished |
| andriy/PPC64_make_fate | success | Make fate finished |
Hello, is there anyone who maintains the "dvvideo" module, who can review this patch and give me some feedback? Greets Mark Am 16.01.21 um 14:40 schrieb Mark Plomer: > Trying to fix https://trac.ffmpeg.org/ticket/8333 > > Some of my old DV AVI files have the DSF-Flag of frames set to 0, > although it is PAL (I think they were rendered with Ulead Media Studio > Pro) ... this causes ffmpeg/VLC-player to produce/play corrupted video. > > In other players/editors it works fine including: > > - VirtualDub > - Windows Media Player > - AVCutty > - Ulead Media Studio Pro (very old) > > I had a look at VirtualDub ... there the PAL/NTSC detection is based > on the frame rate from AVISTREAMINFO header (dwRate/dwScale) - see > https://github.com/Pavuucek/VirtualDub/blob/f47ebd2536f0034b048180d0b9cb9bde0ab10c73/src/VirtualDub/source/VideoSource.cpp#L1211 > > As I don't know, how to access the AVI header info inside > dvvideo_decode_frame()/ff_dv_frame_profile(), I tried another > workaround by checking the buf_size against the dv_profile. > > It works fine now, but I don't know, if this is really the best > solution ... > > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
On Sat, Jan 16, 2021 at 02:40:03PM +0100, Mark Plomer wrote: > Trying to fix https://trac.ffmpeg.org/ticket/8333 > > Some of my old DV AVI files have the DSF-Flag of frames set to 0, although > it is PAL (I think they were rendered with Ulead Media Studio Pro) ... this > causes ffmpeg/VLC-player to produce/play corrupted video. > > In other players/editors it works fine including: > > - VirtualDub > - Windows Media Player > - AVCutty > - Ulead Media Studio Pro (very old) > > I had a look at VirtualDub ... there the PAL/NTSC detection is based on the > frame rate from AVISTREAMINFO header (dwRate/dwScale) - see https://github.com/Pavuucek/VirtualDub/blob/f47ebd2536f0034b048180d0b9cb9bde0ab10c73/src/VirtualDub/source/VideoSource.cpp#L1211 > > As I don't know, how to access the AVI header info inside > dvvideo_decode_frame()/ff_dv_frame_profile(), I tried another workaround by > checking the buf_size against the dv_profile. > > It works fine now, but I don't know, if this is really the best solution ... > > dv_profile.c | 4 ++++ > 1 file changed, 4 insertions(+) > 9676b11bedef27c1c2a359c7617ecc6d30044b16 0001-avcodec-dv_profile-dv-files-with-wrong-dsf-flag.patch > From 9dd240e6700c28601014e79e55d3ddee5f6b13c7 Mon Sep 17 00:00:00 2001 > From: Mark Plomer <not-implemented@mark-plomer.de> > Date: Sat, 16 Jan 2021 14:04:44 +0100 > Subject: [PATCH] avcodec/dv_profile: dv files with wrong dsf flag - detect via > buf_size > > --- The commit message should mention "Fixes Ticket8333" if that is a bugfix for that ticket also maybe some of the other details you write in your mail could be moved into the commit message Thanks [...]
On Sat, 16 Jan 2021, Mark Plomer wrote: > Trying to fix https://trac.ffmpeg.org/ticket/8333 > > Some of my old DV AVI files have the DSF-Flag of frames set to 0, although it > is PAL (I think they were rendered with Ulead Media Studio Pro) ... this > causes ffmpeg/VLC-player to produce/play corrupted video. > > In other players/editors it works fine including: > > - VirtualDub > - Windows Media Player > - AVCutty > - Ulead Media Studio Pro (very old) > > I had a look at VirtualDub ... there the PAL/NTSC detection is based on the > frame rate from AVISTREAMINFO header (dwRate/dwScale) - see > https://github.com/Pavuucek/VirtualDub/blob/f47ebd2536f0034b048180d0b9cb9bde0ab10c73/src/VirtualDub/source/VideoSource.cpp#L1211 > > As I don't know, how to access the AVI header info inside > dvvideo_decode_frame()/ff_dv_frame_profile(), I tried another workaround by > checking the buf_size against the dv_profile. > > It works fine now, but I don't know, if this is really the best solution ... --- a/libavcodec/dv_profile.c +++ b/libavcodec/dv_profile.c @@ -281,6 +281,10 @@ const AVDVProfile* ff_dv_frame_profile(AVCodecContext* codec, const AVDVProfile && codec->coded_height==576) return &dv_profiles[1]; + /* hack for trac issue #8333, dv files with wrong dsf flag - detect via buf_size */ + if (dsf == 0 && stype == dv_profiles[1].video_stype && buf_size == dv_profiles[1].frame_size) + return &dv_profiles[1]; + If possible, then it is probably better to move this fallback to later in the code, right after the hack for trac issue #217, so previous hacks won't get broken... Regards, Marton
Unfortunately it is not possible to move the hack to the bottom, because the "normal processing" (dv_profiles lookup loop) will catch it otherwise (with a wrong profile), because it does not check the frame_size: for (i = 0; i < FF_ARRAY_ELEMS(dv_profiles); i++) if (dsf == dv_profiles[i].dsf && stype == dv_profiles[i].video_stype) return &dv_profiles[i]; But as the new "if" is very specific and catches an "invalid" combination only (with the frame_size check), it should not break anything, I think: if (dsf == 0 && stype == dv_profiles[1].video_stype && buf_size == dv_profiles[1].frame_size) At least, I put the new hack after all other "pre-normal-processing" hacks ;-) Regards Mark Am 13.03.21 um 17:48 schrieb Marton Balint: > > > On Sat, 16 Jan 2021, Mark Plomer wrote: > >> Trying to fix https://trac.ffmpeg.org/ticket/8333 >> >> Some of my old DV AVI files have the DSF-Flag of frames set to 0, >> although it is PAL (I think they were rendered with Ulead Media >> Studio Pro) ... this causes ffmpeg/VLC-player to produce/play >> corrupted video. >> >> In other players/editors it works fine including: >> >> - VirtualDub >> - Windows Media Player >> - AVCutty >> - Ulead Media Studio Pro (very old) >> >> I had a look at VirtualDub ... there the PAL/NTSC detection is based >> on the frame rate from AVISTREAMINFO header (dwRate/dwScale) - see >> https://github.com/Pavuucek/VirtualDub/blob/f47ebd2536f0034b048180d0b9cb9bde0ab10c73/src/VirtualDub/source/VideoSource.cpp#L1211 >> >> As I don't know, how to access the AVI header info inside >> dvvideo_decode_frame()/ff_dv_frame_profile(), I tried another >> workaround by checking the buf_size against the dv_profile. >> >> It works fine now, but I don't know, if this is really the best >> solution ... > > --- a/libavcodec/dv_profile.c > +++ b/libavcodec/dv_profile.c > @@ -281,6 +281,10 @@ const AVDVProfile* > ff_dv_frame_profile(AVCodecContext* codec, const AVDVProfile > && codec->coded_height==576) > return &dv_profiles[1]; > > + /* hack for trac issue #8333, dv files with wrong dsf flag - > detect via buf_size */ > + if (dsf == 0 && stype == dv_profiles[1].video_stype && buf_size > == dv_profiles[1].frame_size) > + return &dv_profiles[1]; > + > > If possible, then it is probably better to move this fallback to later > in the code, right after the hack for trac issue #217, so previous > hacks won't get broken... > > 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-request@ffmpeg.org with subject "unsubscribe".
Oh yes, of course. I updated the commit-message in the attached new patch. Am 13.03.21 um 11:41 schrieb Michael Niedermayer: > > The commit message should mention "Fixes Ticket8333" if that is a bugfix > for that ticket > > also maybe some of the other details you write in your mail could be moved > into the commit message > > Thanks > > [...] >
On Tue, 16 Mar 2021, Mark Plomer wrote: > Unfortunately it is not possible to move the hack to the bottom, because > the "normal processing" (dv_profiles lookup loop) will catch it > otherwise (with a wrong profile), because it does not check the frame_size: Then at least check the 50/60 flag as well in the VAUX source pack, e.g.: pal = !!(frame[80 * 5 + 48 + 3] & 0x20); Regards, Marton > > for (i = 0; i < FF_ARRAY_ELEMS(dv_profiles); i++) > if (dsf == dv_profiles[i].dsf && stype == > dv_profiles[i].video_stype) > return &dv_profiles[i]; > > But as the new "if" is very specific and catches an "invalid" > combination only (with the frame_size check), it should not break > anything, I think: > > if (dsf == 0 && stype == dv_profiles[1].video_stype && buf_size == > dv_profiles[1].frame_size) > > At least, I put the new hack after all other "pre-normal-processing" > hacks ;-) > > Regards > Mark > > Am 13.03.21 um 17:48 schrieb Marton Balint: >> >> >> On Sat, 16 Jan 2021, Mark Plomer wrote: >> >>> Trying to fix https://trac.ffmpeg.org/ticket/8333 >>> >>> Some of my old DV AVI files have the DSF-Flag of frames set to 0, >>> although it is PAL (I think they were rendered with Ulead Media >>> Studio Pro) ... this causes ffmpeg/VLC-player to produce/play >>> corrupted video. >>> >>> In other players/editors it works fine including: >>> >>> - VirtualDub >>> - Windows Media Player >>> - AVCutty >>> - Ulead Media Studio Pro (very old) >>> >>> I had a look at VirtualDub ... there the PAL/NTSC detection is based >>> on the frame rate from AVISTREAMINFO header (dwRate/dwScale) - see >>> > https://github.com/Pavuucek/VirtualDub/blob/f47ebd2536f0034b048180d0b9cb9bde0ab10c73/src/VirtualDub/source/VideoSource.cpp#L1211 >>> >>> As I don't know, how to access the AVI header info inside >>> dvvideo_decode_frame()/ff_dv_frame_profile(), I tried another >>> workaround by checking the buf_size against the dv_profile. >>> >>> It works fine now, but I don't know, if this is really the best >>> solution ... >> >> --- a/libavcodec/dv_profile.c >> +++ b/libavcodec/dv_profile.c >> @@ -281,6 +281,10 @@ const AVDVProfile* >> ff_dv_frame_profile(AVCodecContext* codec, const AVDVProfile >> && codec->coded_height==576) >> return &dv_profiles[1]; >> >> + /* hack for trac issue #8333, dv files with wrong dsf flag - >> detect via buf_size */ >> + if (dsf == 0 && stype == dv_profiles[1].video_stype && buf_size >> == dv_profiles[1].frame_size) >> + return &dv_profiles[1]; >> + >> >> If possible, then it is probably better to move this fallback to later >> in the code, right after the hack for trac issue #217, so previous >> hacks won't get broken... >> >> 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-request@ffmpeg.org with subject "unsubscribe". > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
On Tue, 16 Mar 2021, Marton Balint wrote: > > > On Tue, 16 Mar 2021, Mark Plomer wrote: > >> Unfortunately it is not possible to move the hack to the bottom, because >> the "normal processing" (dv_profiles lookup loop) will catch it >> otherwise (with a wrong profile), because it does not check the frame_size: > > Then at least check the 50/60 flag as well in the VAUX source pack, e.g.: > > pal = !!(frame[80 * 5 + 48 + 3] & 0x20); And as far as I see, the more specific hack checking for codec_tag and similar can be removed then, because your check covers that case as well. Regards, Marton > > Regards, > Marton > > >> >> for (i = 0; i < FF_ARRAY_ELEMS(dv_profiles); i++) >> if (dsf == dv_profiles[i].dsf && stype == >> dv_profiles[i].video_stype) >> return &dv_profiles[i]; >> >> But as the new "if" is very specific and catches an "invalid" >> combination only (with the frame_size check), it should not break >> anything, I think: >> >> if (dsf == 0 && stype == dv_profiles[1].video_stype && buf_size == >> dv_profiles[1].frame_size) >> >> At least, I put the new hack after all other "pre-normal-processing" >> hacks ;-) >> >> Regards >> Mark >> >> Am 13.03.21 um 17:48 schrieb Marton Balint: >>> >>> >>> On Sat, 16 Jan 2021, Mark Plomer wrote: >>> >>>> Trying to fix https://trac.ffmpeg.org/ticket/8333 >>>> >>>> Some of my old DV AVI files have the DSF-Flag of frames set to 0, >>>> although it is PAL (I think they were rendered with Ulead Media >>>> Studio Pro) ... this causes ffmpeg/VLC-player to produce/play >>>> corrupted video. >>>> >>>> In other players/editors it works fine including: >>>> >>>> - VirtualDub >>>> - Windows Media Player >>>> - AVCutty >>>> - Ulead Media Studio Pro (very old) >>>> >>>> I had a look at VirtualDub ... there the PAL/NTSC detection is based >>>> on the frame rate from AVISTREAMINFO header (dwRate/dwScale) - see >>>> >> > https://github.com/Pavuucek/VirtualDub/blob/f47ebd2536f0034b048180d0b9cb9bde0ab10c73/src/VirtualDub/source/VideoSource.cpp#L1211 >>>> >>>> As I don't know, how to access the AVI header info inside >>>> dvvideo_decode_frame()/ff_dv_frame_profile(), I tried another >>>> workaround by checking the buf_size against the dv_profile. >>>> >>>> It works fine now, but I don't know, if this is really the best >>>> solution ... >>> >>> --- a/libavcodec/dv_profile.c >>> +++ b/libavcodec/dv_profile.c >>> @@ -281,6 +281,10 @@ const AVDVProfile* >>> ff_dv_frame_profile(AVCodecContext* codec, const AVDVProfile >>> && codec->coded_height==576) >>> return &dv_profiles[1]; >>> >>> + /* hack for trac issue #8333, dv files with wrong dsf flag - >>> detect via buf_size */ >>> + if (dsf == 0 && stype == dv_profiles[1].video_stype && buf_size >>> == dv_profiles[1].frame_size) >>> + return &dv_profiles[1]; >>> + >>> >>> If possible, then it is probably better to move this fallback to later >>> in the code, right after the hack for trac issue #217, so previous >>> hacks won't get broken... >>> >>> 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-request@ffmpeg.org with subject "unsubscribe". >> _______________________________________________ >> ffmpeg-devel mailing list >> ffmpeg-devel@ffmpeg.org >> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel >> >> To unsubscribe, visit link above, or email >> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Okay cool, I added the check for the PAL flag - this still works fine for my test-videos. I also looked at the old hack above with the codec_tag check - changed in 2013 for ticket #2177 (patch by Michael Niedermayer). I removed that more specific hack and the test-video attached to ticket #2177 also runs into the new hack now ... and still works fine. It also has a "dsf = 0" flag, though it is PAL. So it was really a good idea to consolidate those hacks! Maybe the whole generic matching logic should be refactored, to match against the pal flag instead of the dsf flag? (The "buf_size" check may also be obsolete then). But this will be another task ;-) I attached the updated patch. Regards Mark Am 16.03.21 um 23:12 schrieb Marton Balint: > >> Then at least check the 50/60 flag as well in the VAUX source pack, >> e.g.: >> >> pal = !!(frame[80 * 5 + 48 + 3] & 0x20); > > And as far as I see, the more specific hack checking for codec_tag and > similar can be removed then, because your check covers that case as well.
On Thu, 18 Mar 2021, Mark Plomer wrote: > Okay cool, I added the check for the PAL flag - this still works fine for my > test-videos. > > I also looked at the old hack above with the codec_tag check - changed in > 2013 for ticket #2177 (patch by Michael Niedermayer). I removed that more > specific hack and the test-video attached to ticket #2177 also runs into the > new hack now ... and still works fine. It also has a "dsf = 0" flag, though > it is PAL. > > So it was really a good idea to consolidate those hacks! Maybe the whole > generic matching logic should be refactored, to match against the pal flag > instead of the dsf flag? (The "buf_size" check may also be obsolete then). > But this will be another task ;-) > > I attached the updated patch. It looks good to me, I will apply in a couple of days if nobody comments until then. Also Cc'd Dave, as he got probably the most experience with dv. Regards, Marton
ping ;-) Am 18.03.21 um 21:55 schrieb Marton Balint: > It looks good to me, I will apply in a couple of days if nobody > comments until then. Also Cc'd Dave, as he got probably the most > experience with dv. > > Regards, > Marton
On Tue, 30 Mar 2021, Mark Plomer wrote: > ping ;-) Applied, thanks. Marton > > Am 18.03.21 um 21:55 schrieb Marton Balint: >> It looks good to me, I will apply in a couple of days if nobody >> comments until then. Also Cc'd Dave, as he got probably the most >> experience with dv. >> >> 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-request@ffmpeg.org with subject "unsubscribe".
diff --git a/libavcodec/dv_profile.c b/libavcodec/dv_profile.c index 66505c886b..8f9a358032 100644 --- a/libavcodec/dv_profile.c +++ b/libavcodec/dv_profile.c @@ -281,6 +281,10 @@ const AVDVProfile* ff_dv_frame_profile(AVCodecContext* codec, const AVDVProfile && codec->coded_height==576) return &dv_profiles[1]; + /* hack for trac issue #8333, dv files with wrong dsf flag - detect via buf_size */ + if (dsf == 0 && stype == dv_profiles[1].video_stype && buf_size == dv_profiles[1].frame_size) + return &dv_profiles[1]; + for (i = 0; i < FF_ARRAY_ELEMS(dv_profiles); i++) if (dsf == dv_profiles[i].dsf && stype == dv_profiles[i].video_stype) return &dv_profiles[i];