[FFmpeg-devel] [PATCH 1/3] lavf/riffenc: Improve spec compliance

Sat Mar 12 10:58:01 CET 2016

On Fri, Mar 11, 2016 at 10:26:43PM +0100, Mats Peterson wrote:
> On 03/11/2016 06:45 PM, Michael Niedermayer wrote:
> >On Fri, Mar 11, 2016 at 03:31:55PM +0100, Mats Peterson wrote:
> >>On 03/11/2016 03:27 PM, Mats Peterson wrote:
> >>>On 03/11/2016 03:21 PM, Mats Peterson wrote:
> >>>>On 03/11/2016 03:13 PM, Mats Peterson wrote:
> >>>>>Michael Niedermayer <michael at niedermayer.cc> skrev: (11 mars 2016
> >>>>>15:05:49 CET)
> >>>>>>On Fri, Mar 11, 2016 at 02:12:56PM +0100, Mats Peterson wrote:
> >>>>>>>Mats Peterson <matsp888-at-yahoo.com at ffmpeg.org> skrev: (11 mars 2016
> >>>>>>14:06:19 CET)
> >>>>>>>>Mats Peterson <matsp888-at-yahoo.com at ffmpeg.org> skrev: (11 mars
> >>>>>>2016
> >>>>>>>>13:55:20 CET)
> >>>>>>>>>Michael Niedermayer <michael at niedermayer.cc> skrev: (11 mars 2016
> >>>>>>>>>13:49:32 CET)
> >>>>>>>>>>On Fri, Mar 11, 2016 at 01:28:47PM +0100, Mats Peterson wrote:
> >>>>>>>>>>>On 03/11/2016 01:25 PM, Mats Peterson wrote:
> >>>>>>>>>>>>On 03/11/2016 01:14 PM, Michael Niedermayer wrote:
> >>>>>>>>>>>>>On Fri, Mar 11, 2016 at 05:17:18AM +0100, Mats Peterson wrote:
> >>>>>>>>>>>>>>On 03/11/2016 05:10 AM, Mats Peterson wrote:
> >>>>>>>>>>>>>>>Forget patch 2/3 and 3/3 from the old patch set.
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> From the Microsoft documentation for BITMAPINFOHEADER at
> >>>>>>>>>>>
> >>>>>>>
> >>>>>>>>>>>>>https://msdn.microsoft.com/en-us/library/windows/desktop/dd318229%28v=vs.85%29.aspx:
> >>>>>>>>>>>>>
> >>>>>>>>>>>>>
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>"biSize: Specifies the number of bytes required by the
> >>>>>>>>>structure.
> >>>>>>>>>>This
> >>>>>>>>>>>>>>>value does not include the size of the color table or the
> >>>>>>size
> >>>>>>>>>of
> >>>>>>>>>>the
> >>>>>>>>>>>>>>>color masks, if they are appended to the end of structure."
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>So, biSize is always 40. Also, Windows Media Player won't
> >>>>>>>>detect
> >>>>>>>>>>video
> >>>>>>>>>>>>>>>encoded with Microsoft Video 1 in 8 bpp mode if this value
> >>>>>>is
> >>>>>>>>>>anything
> >>>>>>>>>>>>>>>else than 40. I don't know about other codecs, they probably
> >>>>>>>>>>work.
> >>>>>>>>>>>>>>>Anyway, we should stick with the specs, and not include the
> >>>>>>>>>>palette
> >>>>>>>>>>>>>>>size
> >>>>>>>>>>>>>>>in that field.
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>Regarding the biClrUsed field, I'm setting it to 1 <<
> >>>>>>>>>>>>>>>bits_per_coded_sample if palettized video, since setting it
> >>>>>>to
> >>>>>>>>0
> >>>>>>>>>>is
> >>>>>>>>>>>>>>>another case where it won't work with Windows Media Player
> >>>>>>and
> >>>>>>>>>>>>>>>Microsoft
> >>>>>>>>>>>>>>>Video 1 in 8 bpp mode.
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>Mats
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>>Once, again, HuffYUV has its own variant of BITMAPINFOHEADER
> >>>>>>>>that
> >>>>>>>>>>>>>>*does* include the size of the Huffman tables in biSize.
> >>>>>>That's
> >>>>>>>>>>the
> >>>>>>>>>>>>>>only exception as far as I know.
> >>>>>>>>>>>>>
> >>>>>>>>>>>>>is huffyuv really the exception ? and not raw rgb or palettes
> >>>>>>?
> >>>>>>>>>>>>>
> >>>>>>>>>>>>>asv1 and asv2 do not have 40 there
> >>>>>>>>>>
> >>>>>>>>>>>>>which codec with a non-palette global header puts 40 there ?
> >>>>>>>>>>
> >>>>>>>>>>[...]
> >>>>>>>>>>
> >>>>>>>>>>>As I said before, Microsoft Video 1 in 8-bit mode and RLE4/RLE8
> >>>>>>>>>>>won't even display in Windows Media Player if this value is
> >>>>>>>>anything
> >>>>>>>>>>>else than 40.
> >>>>>>>>>>
> >>>>>>>>>>msv1 / RLE4/8 doesnt have a "Non palette global header"
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>>[...]
> >>>>>>>>>
> >>>>>>>>>That's correct. Sorry. Anyway, the only case I know of is HuffYUV
> >>>>>>with
> >>>>>>>>>its special variant of BITMAPINFOHEADER that includes the Huffman
> >>>>>>>>>tables. Is there stated anywhere in the ASUS specs that the size of
> >>>>>>>>the
> >>>>>>>>>extra data following the BITMAPINFOHEADER should be included in
> >>>>>>>>biSize?
> >>>>>>>>>If not, it should be 40.
> >>>>>>>>>
> >>>>>>>>>Mats
> >>>>>>>>>--
> >>>>>>>>>Mats Peterson
> >>>>>>>>>http://matsp888.no-ip.org/~mats/
> >>>>>>>>>_______________________________________________
> >>>>>>>>>ffmpeg-devel mailing list
> >>>>>>>>>ffmpeg-devel at ffmpeg.org
> >>>>>>>>>http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >>>>>>>>
> >>>>>>>>Those asv1/asv2 files play just fine with a biSize of 40, at that.
> >>>>>>>>
> >>>>>>>>Mats
> >>>>>>>>--
> >>>>>>>>Mats Peterson
> >>>>>>>>http://matsp888.no-ip.org/~mats/
> >>>>>>>>_______________________________________________
> >>>>>>>>ffmpeg-devel mailing list
> >>>>>>>>ffmpeg-devel at ffmpeg.org
> >>>>>>>>http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >>>>>>>
> >>>>>>>Should we perhaps "tighten" the "specs" (I saw that you have written
> >>>>>>some documentation on asv1/asv2 in FFmpeg), and include those extra
> >>>>>>bytes in biSize? I suppose this is somewhat of a proprietary FFmpeg
> >>>>>>invention.
> >>>>>>
> >>>>>>we didnt invent ASUS video 1 and 2 nor huffyuv
> >>>>>>
> >>>>>>which codec with a non-palette global header puts 40 there ?
> >>>>>>
> >>>>>>the ms specs just says the color table & mask isnt part of biSize
> >>>>>>it doesnt say the global header isnt, or iam looking at the wrong text
> >>>>>>or location in the text/spec
> >>>>>>
> >>>>>>[...]
> >>>>>
> >>>>>Again, only one I know of is HuffYUV, and the specs define a custom
> >>>>>BITMAPINFOHEADER that includes the Huffman tables. The "global data"
> >>>>>in ASUS is only 8 bytes. Do they regard the BITMAPINFOHEADER as being
> >>>>>part of this global data as well? And should we? A BITMAPINFOHEADER is
> >>>>>a BITMAPINFOHEADER is a BITMAPINFOHEADER...
> >>>>>
> >>>>>Mats
> >>>>>
> >>>>
> >>>>Do you have a sample file with asv1/asv2 *created by ASUS* where biSize
> >>>>isn't 40?
> >>>>
> >>>>Mts
> >>>>
> >>>
> >>>I got the asv1/asv2 samples from the MPlayer sample directory. Are they
> >>>from ASUS? They all use a biSize of 48.
> >>>
> >>>Mats
> >>>
> >>
> >>Nope. Written with libavformat.
> >
> >why do you think they are written by libavformat ?
> >
> >i see:
> >"C:\PROGRAM FILES\ASUS\ASUS LIVE\ASUSLIVE.EXE -AVICAP32- ASUS Video Capture Driver, Version:  3.8.2.2"
> >
> >in asv2_320x240_3.avi
> >
> >
> 
> Yes, you are right. I was looking at some other files. Sorry. So how
> do we proceed from here? Should we make an exception to the "40-byte
> rule" for HuffYUV, ffvhuff, asv1 and asv2 in the meantime?

before writing a system based on a list of exceptions, please awnser
the question

which codec with a non-palette global header puts 40 there ?

if the awnser is none then the existence of a palette in extradata
is what decides the biSize value
and a AV_CODEC_PROP_EXTRADATA_IS_PALETTE can then be added to the
codecs affected. Thats alot cleaner than a list in riffenc which
people will forget to update, especially if that list lists all
the modern codecs which will grow instead of historic paletted
codecs which will likely not grow much

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Rewriting code that is poorly written but fully understood is good.
Rewriting code that one doesnt understand is a sign that one is less smart
then the original author, trying to rewrite it will not make it better.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20160312/eb2bc817/attachment.sig>