[FFmpeg-devel] [FFmpeg-cvslog] mpegaudiodec_template: disable CRC checking for layers 1 and 2

[Michael Niedermayer]

On Mon, Aug 03, 2020 at 10:47:44AM +0200, Lynne wrote:
> Aug 3, 2020, 01:45 by michael at niedermayer.cc:
> 
> > On Sun, Aug 02, 2020 at 08:52:22PM +0000, Lynne wrote:
> >
> >> ffmpeg | branch: master | Lynne <dev at lynne.ee> | Sun Aug  2 22:45:00 2020 +0200| [b48397e7b84864f2d4c70361a4c4bed93e826753] | committer: Lynne
> >>
> >> mpegaudiodec_template: disable CRC checking for layers 1 and 2
> >>
> >> Layers 1 and 2 use lengths in bits which are not a multiple of 8,
> >> and our CRC works on a per-byte basis.
> >>
> >> > http://git.videolan.org/gitweb.cgi/ffmpeg.git/?a=commit;h=b48397e7b84864f2d4c70361a4c4bed93e826753
> >> ---
> >>
> >>  libavcodec/mpegaudiodec_template.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >
> > Please send patches before pushing changes like everyone else
> > it was just luck that i spoted this change
> >
> > there quite possibly might be people who would want to fix this instead
> > of disabling it.
> >
> > I for example would certainly want to take a look if i knew there was
> > a problem and had a testcase (when i have time)
> >
> > thx
> >
> 
> This actually broke decoding if both crccheck and explode were set and I broke it,
> so I think fixing this quickly first is better. No one else has been interested in CRC checks
> after all and I only noticed this because I always run my audio decoder with those settings.
> I did attempt to fix it myself, but like the commit comment said, it requires feeding the
> CRC bits instead of bytes for some inputs, as that's what the MPEG-1 spec says,
> and that's also what libtwolame seems to generate for 2ch 48khz inputs and 128kbps out.
> 

> If you'd like to try fixing it properly you can generate an mp2 file with error protection via
> -c:a libtwolame -error_protection 1 and decode via ffmpeg -err_detect +crccheck <input>

just tried and it seems working with my patch


> The CRC is otherwise the same, it still reads 16 bits at the start, skips, and reads N bits,
> its just the number N that changed. ISO 11172-3 Annex B defines the numbers,

I thought so too, but that didnt work


> you can find it at http://read.pudn.com/downloads72/doc/260415/11172/11172_3_ANNEXAB.DOC
> Look for the table "NUMBER OF PROTECTED AUDIO_DATA BITS". You can open the
> doc with any code editor, it isn't a binary word doc, just uses some pseudo-markdown syntax.
> 

> Reading bits for CRC isn't a common case, but I've encountered this before when trying
> to make the AAC-HE decoder check the CRC, so maybe having a function in libavcodec or
> libavutil for that would be a good idea.

yes though in the patch posted ive implemented it by transforming the 
non byte input into a multiple of 8 bits.
Just saying so noone is confused by what the code does ...

That could be moved to a common function or some bitwise CRC loop based
one could be addded

thx

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

The greatest way to live with honor in this world is to be what we pretend
to be. -- Socrates
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20200803/bae1f7ce/attachment.sig>