Differential D11634

K3b FFMpeg Decoder Plugin Fix
ClosedPublic
Actions

Authored by blaze on Mar 24 2018, 7:17 AM.

Details

Reviewers
lesliezhai
Commits
R467:059d0da2e2c6: [plugins] K3b FFMpeg Decoder Plugin Fix
Summary

Here's my proposal to fix the https://bugs.kde.org/show_bug.cgi?id=386105
See my comments there to get the idea of what's wrong with the plugin.
These formats I've enabled (APE and WavPack) were the actual formats I was testing against.

Diff Detail

Repository
R467 K3b
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
blaze requested review of this revision.Mar 24 2018, 7:17 AM
blaze created this revision.

I'll probably need the commit access to commit these changes

lesliezhai added a comment.Mar 24 2018, 10:49 AM

Hi Mikołaj,

Thanks for your contribution!

Could you please rebase your patch, because:

$ patch -Np1 -i D11634.diff 
patching file plugins/decoder/ffmpeg/k3bffmpegwrapper.cpp
Hunk #4 FAILED at 318.
Hunk #5 succeeded at 356 (offset 3 lines).
Hunk #6 succeeded at 408 (offset 3 lines).
Hunk #7 succeeded at 491 (offset 3 lines).
1 out of 7 hunks FAILED -- saving rejects to file plugins/decoder/ffmpeg/k3bffmpegwrapper.cpp.rej

Please git pull from upstream remote:

$ git remote -v
origin  git@git.kde.org:k3b (fetch)
origin  git@git.kde.org:k3b (push)

$ git log
commit 74f8b65981342a1240e7ac6610107e906dc0b884
Author: Albert Astals Cid <aacid@kde.org>
Date:   Mon Mar 19 23:53:26 2018 +0100

    GIT_SILENT Upgrade KDE Applications version to 18.07.70.

Regarads,
Leslie Zhai

blaze updated this revision to Diff 30388.Mar 24 2018, 1:18 PM

Rebased to the current master

lesliezhai accepted this revision.Mar 25 2018, 1:31 AM

Hi Mikołaj,

LGTM, please commit it, thanks!

Regards,
Leslie Zhai

This revision is now accepted and ready to land.Mar 25 2018, 1:31 AM
anthonyfieroni added a subscriber: anthonyfieroni.Mar 25 2018, 4:49 AM
anthonyfieroni added inline comments.
plugins/decoder/ffmpeg/k3bffmpegwrapper.cpp
325

This should be delete[]

331

What about here, it advance on dangling pointer?

blaze updated this revision to Diff 30467.Mar 25 2018, 6:57 AM

delete -> delete[]

blaze updated this revision to Diff 30478.Mar 25 2018, 9:44 AM
blaze marked an inline comment as done.

Check if it's safe to deallocate the buffer and to move the pointer

blaze updated this revision to Diff 30699.EditedMar 27 2018, 7:42 AM
blaze marked an inline comment as done.
blaze changed the visibility from "Public (No Login Required)" to "All Users".
blaze removed a subscriber: anthonyfieroni.

Hello. I decided to rework the patch since some important changes happened:

  1. I realized that k3b only supports 16bit audio (which is reasonable, because it was created to work mainly with CD audio), therefore removed some excess code;
  2. disabled WMA and AAC because they're floating point formats and need to be converted to 16 bit, which I'm planning to add eventually in the future.

Leslie, please review these changes again and then commit on my behalf as I have no write access atm.

blaze updated this revision to Diff 30713.EditedMar 27 2018, 12:40 PM

Final revision.
UPD: not really :D

lesliezhai added a comment.Mar 31 2018, 7:49 AM

Hi Mikołaj,

LGTM! Do you have commit access or should I commit it on your behalf?

PS: you can implement that in the next patch to use libswresample for covering all possible cases for the input audio.

Regards,
Leslie Zhai

blaze updated this revision to Diff 31153.EditedApr 2 2018, 3:42 PM

After my research on libswresample topic I started to think that it's not a good solution for this situation, quite the opposite even. It still requires format conversions which renders the code to a much more complex state. So no advantages here really.

So I updated the code with my previous solution for the floating point format audio.

It's time to sum up the work: this patch fixes 16 bit audio decoding, adds planar formats support (WavPack and APE in particular), introduces floating point audio support (WMA, AAC).

Leslie, please review this change and commit on my behalf

Closed by commit R467:059d0da2e2c6: [plugins] K3b FFMpeg Decoder Plugin Fix (authored by lesliezhai). · Explain WhyApr 3 2018, 2:49 PM
This revision was automatically updated to reflect the committed changes.
ltoscano added a subscriber: ltoscano.Apr 27 2018, 3:05 PM

Leslie, please make sure to commit using the right authorship information for the author of the patch, not yours.

ltoscano changed the visibility from "All Users" to "Public (No Login Required)".Apr 27 2018, 3:07 PM

Revision Contents
Changeset List

Diff 31220

View Options

plugins/decoder/ffmpeg/k3bffmpegwrapper.cpp

Loading...