[FFmpeg-devel] [PATCH v3 1/1] fftools/ffprobe: print size of attachment streams (extradata_size)

Sat Nov 27 17:57:02 EET 2021

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of Michael
> Niedermayer
> Sent: Saturday, November 27, 2021 3:13 PM
> To: FFmpeg development discussions and patches <ffmpeg-devel at ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH v3 1/1] fftools/ffprobe: print size of
> attachment streams (extradata_size)
> 
> On Sat, Nov 27, 2021 at 01:12:31PM +0000, Soft Works wrote:
> >
> >
> > > -----Original Message-----
> > > From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of Michael
> > > Niedermayer
> > > Sent: Saturday, November 27, 2021 1:19 PM
> > > To: FFmpeg development discussions and patches <ffmpeg-devel at ffmpeg.org>
> > > Subject: Re: [FFmpeg-devel] [PATCH v3 1/1] fftools/ffprobe: print size of
> > > attachment streams (extradata_size)
> > >
> > > On Sat, Nov 27, 2021 at 12:58:30AM +0100, Michael Niedermayer wrote:
> > > > On Fri, Nov 26, 2021 at 10:01:23PM +0000, Soft Works wrote:
> > > > >
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: ffmpeg-devel <mailto:ffmpeg-devel-bounces at ffmpeg.org> On
> Behalf Of
> > > Michael
> > > > > > Niedermayer
> > > > > > Sent: Friday, November 26, 2021 10:26 PM
> > > > > > To: FFmpeg development discussions and patches <mailto:ffmpeg-
> devel at ffmpeg.org>
> > > > > > Subject: Re: [FFmpeg-devel] [PATCH v3 1/1] fftools/ffprobe: print
> size
> > > of
> > > > > > attachment streams (extradata_size)
> > > > > >
> > > > > > On Fri, Nov 26, 2021 at 07:55:40PM +0000, Soft Works wrote:
> > > > > > >
> > > > > > >
> > > > > > > > -----Original Message-----
> > > > > > > > From: ffmpeg-devel <mailto:ffmpeg-devel-bounces at ffmpeg.org> On
> Behalf Of
> > > Michael
> > > > > > > > Niedermayer
> > > > > > > > Sent: Friday, November 26, 2021 7:50 PM
> > > > > > > > To: FFmpeg development discussions and patches <mailto:ffmpeg-
> devel at ffmpeg.org>
> > > > > > > > Subject: Re: [FFmpeg-devel] [PATCH v3 1/1] fftools/ffprobe:
> print
> > > size of
> > > > > > > > attachment streams (extradata_size)
> > > > > > > >
> > > > > > > > On Thu, Nov 25, 2021 at 04:59:41PM +0000, Soft Works wrote:
> > > > > > > > > Another attempt: Created on Linux and zipped...
> > > > > > > >
> > > > > > > > tested and works
> > > > > > > > LGTM
> > > > > > >
> > > > > > > Thanks.
> > > > > > >
> > > > > > > Today I've seen that the v3 version with the patch as an
> attachment
> > > has
> > > > > > > been processed by patchwork:
> > > > > > >
> > > > > > >
> > > > > >
> > >
> https://patchwork.ffmpeg.org/project/ffmpeg/patch/CH0P223MB03639F30A548FA85C1
> > > > > > mailto:7E8855BA629 at CH0P223MB0363.NAMP223.PROD.OUTLOOK.COM/
> > > > > > >
> > > > > > > I've also verified that the output from git format-patch on Linux
> was
> > > the
> > > > > > > same as on Windows. Also, I sent e-mails with the attached patch
> to
> > > myself
> > > > > > > (google-to-google and google-to-ms) and verified that the
> attachments
> > > were
> > > > > > > unchanged.
> > > > > > >
> > > > > > > My preliminary conclusion for patches containing long lines:
> > > > > > >
> > > > > > > For patchwork: Use the --attach param for git send-patch
> > > > > > > For Michael and maybe others: send as zip attachment 😊
> > > > > > > (not that I could tell the reason)
> > > > > > >
> > > > > > > Might be interesting to check whether there's a difference
> between
> > > the
> > > > > > > e-mail you get and the file you get when downloading the mbox
> from
> > > > > > patchwork.
> > > > > >
> > > > > > The attachemnt after the mailing list is broken:
> > > > > > https://ffmpeg.org/pipermail/ffmpeg-
> > > > > > devel/attachments/20211125/daa80b4e/attachment.bin
> > > > > >
> > > > > > if you want to help fix this, thats welcome
> > > > > >
> > > > > > That patchwork has a working patch is interresting but iam not sure
> if
> > > the
> > > > > > awnser to this will help, the problem seems before. So we need a
> fix
> > > before
> > > > > > if you are interrested in helping, there was some prior occurance
> of
> > > this
> > > > > > maybe there was some additional information in that thread
> > > > >
> > > > > There are two problems - I'm not sure which one you are talking
> about:
> > > >
> > > > theres a problem which has occured previously, for which there is one
> > > > or more threads on the ML
> > > > if you want to help, find these threads. Throwing out ideas which are
> > > > not even consistent with the current facts causes people like me to
> > > > waste time looking at the wrong places.
> > > > Let alone the problem was not that we didnt know where the bug was
> > > > IIRC it was that someone had to fix it
> > > > It is true that the new instance here has some currently unexplained
> > > > behaviours but neither your 1. or 2. works at explaining them
> > > >
> > > >
> > > > [...]
> > > >
> > > > > 2. You didn't correctly receive the patch sent as attachment
> > > > >
> > > > > I just double-checked: The ML software didn't modify the patch when
> > > attached
> > > > > (as plain-text .patch)
> > > >
> > > > how did you check that ?
> > > > it is broken on my side and it is broken on the server as you can see
> by
> > > > looking at the link i posted
> > > > Also i mailed the correct patch to myself as plain/text attachment and
> > > > its perfectly fine. Neither my SMTP server not my local mail handling
> > > > did anything odd to the long line
> > > > OTOH the mail i received from the ML though contains a base64 encoded
> > > > attachment and inside that is a broken patch
> > > > if you say you have received a good one then my first intuition is to
> > > > ask you to check that again
> > > > i took the attachment decoded it with command line base64 -d and it
> > > > was not good
> > > >
> > > >
> > > > >
> > > > > Patchwork could apply this correctly, so I'm not sure what went wrong
> > > > > in that case when you couldn't apply it?
> > > >
> > > > Your attachment is broken on the server already, again
> > > > your mail with the attachment:
> > > > https://lists.ffmpeg.org/pipermail/ffmpeg-devel/2021-
> November/288260.html
> > > > and the attachment:
> > > > https://ffmpeg.org/pipermail/ffmpeg-
> devel/attachments/20211125/daa80b4e/attachment.bin
> > > > a954c4f6237453e91ab3e0327ec5eae9  attachment.bin <-- this is the broken
> one
> > > >
> > > > patch --dry-run -p1 <attachment.bin
> > > > checking file doc/ffprobe.xsd
> > > > checking file fftools/ffprobe.c
> > > > Hunk #1 succeeded at 2769 (offset -14 lines).
> > > > checking file tests/ref/fate/concat-demuxer-extended-lavf-mxf
> > > > checking file tests/ref/fate/concat-demuxer-extended-lavf-mxf_d10
> > > > checking file tests/ref/fate/concat-demuxer-simple1-lavf-mxf
> > > > checking file tests/ref/fate/concat-demuxer-simple1-lavf-mxf_d10
> > > > checking file tests/ref/fate/concat-demuxer-simple2-lavf-ts
> > > > checking file tests/ref/fate/flv-demux
> > > > checking file tests/ref/fate/mov-zombie
> > > > checking file tests/ref/fate/mxf-probe-d10
> > > > checking file tests/ref/fate/oggopus-demux
> > > > checking file tests/ref/fate/ts-demux
> > > > checking file tests/ref/fate/ts-opus-demux
> > > > checking file tests/ref/fate/ts-small-demux
> > > > patch unexpectedly ends in middle of line
> > > > Hunk #1 FAILED at 145.
> > > > 1 out of 1 hunk FAILED
> > > >
> > > > you can just try to download it from the web archieve and
> > > > try for yourself its broken before my mail stuff ever sees it
> > > > and that is not inline
> > > > Neither your case 1. nor 2. explain this and this is a bug
> > > > on the server somewhere, i dont remember where but i think i
> > > > knew it previosuly what caused this
> > > > something along the postfix - mailman chain probably
> > > > i also faintly remeber considering to simply bump the 998
> > > > limit up to workaround it
> > > > maybe anton or someone else remembers more details
> > > > I sadly dont have enough time ATM to really re-investigate this
> > >
> > > I had a bit more time to look into this
> > >
> > > this issue may be unrelated to the 998 line limit, the patch on this mail
> > > is simply corrupted
> > > no matter if i take it from the received mail or the mailman archive
> > >
> > > patch --dry-run -p1 <~/v3-0001-fftools-ffprobe-print-size-of-attachment-
> > > streams-.patch
> > > ...
> > > checking file tests/ref/fate/ts-small-demux
> > > patch unexpectedly ends in middle of line
> > > Hunk #1 FAILED at 145.
> > > 1 out of 1 hunk FAILED
> > >
> > > but with -F9
> > > patch --dry-run -p1 -F9  <~/v3-0001-fftools-ffprobe-print-size-of-
> attachment-
> > > streams-.patch
> > > checking file tests/ref/fate/ts-small-demux
> > > patch unexpectedly ends in middle of line
> > > Hunk #1 succeeded at 145 with fuzz 3.
> > >
> > > This 2nd case is likely how patchwork could apply the patch
> > >
> > > the corruption is a missing crlf, if i add it by hand it works without -
> F9
> > >
> > > if i apply it locally and rebuild the patch with format-patch
> > >
> > > git format-patch -1
> > > git checkout HEAD^
> > > patch --dry-run -p1 <0001-Try.patch
> > > it works fine
> >
> >
> > That’s a really tricky case, because v2 of the patch (sent inline) had
> > in fact incorrect line breaks (see screenshot).
> >
> > Then I sent the patch as an attachment (v3) and that in turn had the effect
> that
> > the last line is ending without a line break.
> >
> > Finally I sent the whole patch as a zipped attachment - which worked (of
> course),
> > and the patch you created locally works as well (not surprising).
> >
> > But there are still two problems:
> >
> 
> > 1. Patches sent to the ML as inline get long lines truncated
> 
> This should not be surprising, the SMTP RFC recommands this truncation
> maybe this could be avoided if either mailman or postfix encode the message
> as base64 either always or conditional on long lines
> if someone who knows how to do this reads this, please explain. But keep in
> mind maintaining code changes in mailman is something i want to avoid so
> future security updates stay easy
> Also i assume that inline mail wasnt base64 when it was received, i didnt
> check, if it already was then i guess that wouldnt help
> The alternative is to make postfix not do this truncation and ignore the RFC
> that is easy. But do we want that ? (again i assume here it was postfix
> which did this truncation i did not confirm this)
> 
> 
> > 2. Patches sent as attachment are missing a final line break
> >
> > for 1., I'm sure it's a universal problem for everybody.
> 
> > for 2., I'm wondering whether it's just my E-Mail client which removes that
> last
> > line break when sending or whether that happens somewhere else...
> 
> as i apply alot of patches and this isnt normally happening the question
> arrises
> what was different here. It was a patch attached by you.

I didn't attach it. I ran 'git format-patch' with the --attach parameter.
The result was a message/patch where the commit is included as an attachment.

But now I did another test: I installed Thunderbird and opened the generated 
e-mail locally: the trailing LF was preserved.
Not so with Outlook: it truncates the attachment when saving (and probably
when sending), so the final LF gets dropped.

Then I changed the line endings of the generated patch from LF to CRLF,
opened it in Outlook, saved the attachment -> this time the final line 
break was preserved.

OK, culprit found, not yet a solution.

Thanks for helping me analyze this,
softworkz