[FFmpeg-devel] [PATCH] avformat/avidec: ensure that palette does not contain the BottomUp info.

[Michael Niedermayer]

On Wed, Sep 24, 2014 at 08:57:31AM +0200, Benoit Fouet wrote:
> Hi,
> 
> ----- Mail original -----
> > On Tue, Sep 23, 2014 at 03:11:58PM +0200, Michael Niedermayer wrote:
> > > On Tue, Sep 23, 2014 at 01:33:30PM +0200, Benoit Fouet wrote:
> > > > Hi,
> > > > 
> > > > ----- Mail original -----
> > > > > Hi,
> > > > > 
> > > > > ----- Mail original -----
> > > > > > On Tue, Sep 23, 2014 at 10:19:17AM +0200, Benoit Fouet wrote:
> > > > > 
> > > > > > > Here is a patch to illustrate this...
> > > > > > 
> > > > > > this looks better but:
> > > > > > 
> > > > > > 
> > > > > > [...]
> > > > > > > -                ff_put_bmp_header(pb, enc,
> > > > > > > ff_codec_bmp_tags, 0,
> > > > > > > 0);
> > > > > > > +                if (keep_height) {
> > > > > > > +                    enc->extradata_size -= 9;
> > > > > > > +                    if (!enc->extradata_size)
> > > > > > > +                        av_freep(&enc->extradata);
> > > > > > > +                }
> > > > > > 
> > > > > > what if extradata is stored in 2 files, the 2nd muxer would
> > > > > > no
> > > > > > longer
> > > > > > see it
> > > > > > also the muxer shouldnt change the encoders/demuxer extradata
> > > > > > 
> > > > > > and cant all this logic be put in ff_put_bmp_header() ?
> > > > > > might even be simpler
> > > > > > 
> > > > > 
> > > > > Do you mean that ff_put_bmp_header() would check for the
> > > > > BottomUp
> > > > > presence and not negate height in this case, and also only
> > > > > write the
> > > > > palette in the stream?
> > > > > 
> > > > 
> > > > Patch attached, just in case this is what you meant...
> > > 
> > > >  riffenc.c |    6 ++++--
> > > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > > > d3a93efc5680ad2340086a61ba3d59749f67e770
> > > >  0001-avformat-riffenc-extend-ff_put_bmp_header-to-be-able.patch
> > > > From b8c8f07555769215af7a10c48e7a479f783a7c49 Mon Sep 17 00:00:00
> > > > 2001
> > > > From: Benoit Fouet <benoit.fouet at free.fr>
> > > > Date: Tue, 23 Sep 2014 10:07:10 +0200
> > > > Subject: [PATCH] avformat/riffenc: extend ff_put_bmp_header to be
> > > > able to
> > > >  force height to not be changed.
> > > > 
> > > > When stream copying, height shouldn't be changed when writing the
> > > > BITMAPINFOHEADER header if the BottomUp information is present in
> > > > the
> > > > extradata.
> > > > ---
> > > >  libavformat/riffenc.c | 6 ++++--
> > > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/libavformat/riffenc.c b/libavformat/riffenc.c
> > > > index ef4d399..f1a2274 100644
> > > > --- a/libavformat/riffenc.c
> > > > +++ b/libavformat/riffenc.c
> > > > @@ -209,11 +209,13 @@ int ff_put_wav_header(AVIOContext *pb,
> > > > AVCodecContext *enc, int flags)
> > > >  void ff_put_bmp_header(AVIOContext *pb, AVCodecContext *enc,
> > > >                         const AVCodecTag *tags, int for_asf, int
> > > >                         ignore_extradata)
> > > >  {
> > > > +    int keep_height = enc->extradata_size >= 9 &&
> > > > +                      !memcmp(enc->extradata +
> > > > enc->extradata_size - 9, "BottomUp", 9);
> > > >      /* size */
> > > >      avio_wl32(pb, 40 + (ignore_extradata ? 0 :
> > > >      enc->extradata_size));
> > > >      avio_wl32(pb, enc->width);
> > > >      //We always store RGB TopDown
> > > > -    avio_wl32(pb, enc->codec_tag ? enc->height : -enc->height);
> > > > +    avio_wl32(pb, enc->codec_tag || keep_height ? enc->height :
> > > > -enc->height);
> > > >      /* planes */
> > > >      avio_wl16(pb, 1);
> > > >      /* depth */
> > > 
> > > > @@ -227,7 +229,7 @@ void ff_put_bmp_header(AVIOContext *pb,
> > > > AVCodecContext *enc,
> > > >      avio_wl32(pb, 0);
> > > >  
> > > >      if (!ignore_extradata) {
> > > > -        avio_write(pb, enc->extradata, enc->extradata_size);
> > > > +        avio_write(pb, enc->extradata, keep_height ?
> > > > enc->extradata_size : enc->extradata_size - 9);
> > > 
> > > this corrupts the stored extradata and breaks fate
> > 
> > fixed and applied
> > 
> > thanks
> > 
> 
> Yes, I swapped the ternary...
> Also, do you think it makes sense to revert my previous patch?

id like to keep it so we also support files that where remuxed with
the previous buggy code

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The real ebay dictionary, page 1
"Used only once"    - "Some unspecified defect prevented a second use"
"In good condition" - "Can be repaird by experienced expert"
"As is" - "You wouldnt want it even if you were payed for it, if you knew ..."
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20140924/d03e32e5/attachment.asc>