melchor629 / node-flac-bindings

Nodejs bindings to libFLAC
ISC License
17 stars 1 forks source link

FLAC compressionLevel parameter is ignored #6

Closed dmooney65 closed 7 years ago

dmooney65 commented 7 years ago

Hello again!

Low priority this one but nice to have.

  1. compressionLevel option is ignored - compression level 0 is always used.

  2. Default compression level for FLAC should be 5, but the bindings are using level 0.

Verified using flac command line tool on Ubuntu 16.04 vs bindings output.

Thanks

melchor629 commented 7 years ago

Oh my gosh, compressionLevel is ignored totally. I will check that and other parameters if they're ignored.

But the default compression level literally is not set never, and the default value is null (that means it won't be set). I will investigate about the second.

melchor629 commented 7 years ago

Using default values, the function FLAC__stream_encoder_set_compression_level is called only two times:

  1. When the encoder is created (the value is 5)
  2. When the encoder is going to be destroyed (the value is again 5)

You can see the debug session I have just done:

(lldb) r
Process 4216 launched: '/usr/local/bin/node' (x86_64)
Process 4216 stopped
* thread #1, queue = 'com.apple.main-thread', stop reason = breakpoint 1.1
    frame #0: 0x0000000103bc563c libFLAC.dylib`FLAC__stream_encoder_set_compression_level
libFLAC.dylib`FLAC__stream_encoder_set_compression_level:
->  0x103bc563c <+0>: pushq  %rbp
    0x103bc563d <+1>: movq   %rsp, %rbp
    0x103bc5640 <+4>: pushq  %r15
    0x103bc5642 <+6>: pushq  %r14
Target 0: (node) stopped.
(lldb) print $rsi
(unsigned long) $2 = 5
(lldb) up
frame #1: 0x0000000103bc2bb5 libFLAC.dylib`FLAC__stream_encoder_new + 141
libFLAC.dylib`FLAC__stream_encoder_new:
    0x103bc2bb5 <+141>: movq   0x8(%r15), %rdx
    0x103bc2bb9 <+145>: movl   $0x0, 0x35c8(%rdx)
    0x103bc2bc3 <+155>: movl   $0x638, %eax              ; imm = 0x638
    0x103bc2bc8 <+160>: xorl   %ecx, %ecx
(lldb) c
Process 4216 resuming
Process 4216 stopped
* thread #1, queue = 'com.apple.main-thread', stop reason = breakpoint 1.1
    frame #0: 0x0000000103bc563c libFLAC.dylib`FLAC__stream_encoder_set_compression_level
libFLAC.dylib`FLAC__stream_encoder_set_compression_level:
->  0x103bc563c <+0>: pushq  %rbp
    0x103bc563d <+1>: movq   %rsp, %rbp
    0x103bc5640 <+4>: pushq  %r15
    0x103bc5642 <+6>: pushq  %r14
Target 0: (node) stopped.
(lldb) print $rsi
(unsigned long) $3 = 5
(lldb) up
frame #1: 0x0000000103bc37de libFLAC.dylib`FLAC__stream_encoder_finish + 2074
libFLAC.dylib`FLAC__stream_encoder_finish:
    0x103bc37de <+2074>: testl  %r14d, %r14d
    0x103bc37e1 <+2077>: jne    0x103bc37ed               ; <+2089>
    0x103bc37e3 <+2079>: movq   (%r12), %rax
    0x103bc37e7 <+2083>: movl   $0x1, (%rax)
(lldb) c
Process 4216 resuming
Process 4216 exited with status = 0 (0x00000000)

I will investigate further, but it's true that with default values, the compression level is lower than expected with a value of 5.

melchor629 commented 7 years ago

After this 45 minutes, I made some tests and I wrote a demo program in C to compare the outputs when using the default value of compression and when using a custom one.

https://gist.github.com/melchor629/67be65e2f7c1f42deb9064bdc4805401

I think this problem is not about the bindings, probably is from the FLAC library itself. It seems that the compression level must be explicitly called, if not will use 0, instead of 5. But as a workaround, I will set as a default value 5 instead of null to force the bindings to use 5 value always and, of course, making the output a bit more compressed.

I don't know why the C program has an output size higher than the bindings, the duration of both flacs are exactly the same 🤔

dmooney65 commented 7 years ago

OK great - will give it a try.

Can't help you with the file sizes - wierd.

Thanks again

melchor629 commented 7 years ago

@dmooney65 The file sizes is only a weird thing to comment :) Should work now with the workaround.