Use embedded cover in MP4 video files
AbandonedPublic

Authored by hschaefer on Aug 15 2019, 8:23 AM.

Details

Summary

BUG: 408956

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped
hschaefer requested review of this revision.Aug 15 2019, 8:23 AM
hschaefer created this revision.
cfeck added a reviewer: bruns.Aug 15 2019, 2:33 PM
cfeck retitled this revision from Bug 408956 - Embedded cover in MP4 (edit) to Use embedded cover in MP4 video files.Aug 15 2019, 2:35 PM
cfeck edited the summary of this revision. (Show Details)
cfeck added a subscriber: Dolphin.
cfeck added a subscriber: cfeck.
cfeck added inline comments.
ffmpegthumbnailer/moviedecoder.cpp
253

If this variable isn't needed, just remove this line.

meven added a subscriber: meven.Aug 15 2019, 5:43 PM
meven added inline comments.
ffmpegthumbnailer.cpp
50

I hope this kind of check is not too heavy, or we might to check the file mimetype before.

52

Missing space after if

59

Same

64

Same

Since I never used phabricator before, who needs to make the changes to the patch? Me, the submitter of the patch, or the one who finally integrates the patch?

hschaefer added inline comments.Aug 15 2019, 10:35 PM
ffmpegthumbnailer.cpp
51

A quick glance at the source code of taglib revealed that in isValid() are 2 checks are done

  1. check if there are atoms at alll
  2. check if there is a "moov" atom

I have directories with more than 50 MP4s all including covers and it is reasonable fast. Defintely faster than extracting the same amount of still frames out of the video.

Since I never used phabricator before, who needs to make the changes to the patch? Me, the submitter of the patch, or the one who finally integrates the patch?

You :)

hschaefer updated this revision to Diff 63849.Aug 16 2019, 5:37 AM
  • Spaces after if
  • removed unused, commented out variable
hschaefer marked 4 inline comments as done.Aug 16 2019, 5:37 AM

Thanks for the patch. Please read https://community.kde.org/Policies/Commit_Policy about how to write a good commit message.

CMakeLists.txt
6–21

Please add a cmake/FindTaglib.cmake file (you can re-use https://cgit.kde.org/kio-extras.git/tree/cmake/FindTaglib.cmake) and then use find_package(Taglib) instead of requiring pkg-config.

ffmpegthumbnailer/filmstripfilter.h
29

Is this change related?

ffmpegthumbnailer/moviedecoder.cpp
253–254

Is this change related? If not, we should do it in another commit.

@elvisangelaccio maybe we should move FindTaglib.cmake into ECM. Quite a few projects seem to have their own copies of it.

@elvisangelaccio maybe we should move FindTaglib.cmake into ECM. Quite a few projects seem to have their own copies of it.

See D21695

hschaefer updated this revision to Diff 65964.Sep 13 2019, 9:36 AM

Copied https://cgit.kde.org/kio-extras.git/tree/cmake/FindTaglib.cmake and used it to get rid of pkgconfig-depency

Hello,

I just watched into the current git. Is my patch rejected?

ngraham requested changes to this revision.Jun 10 2020, 3:18 PM

We should probably just do this now and then remove the redundant copies of FindTagLib later.

@elvisangelaccio?

This revision now requires changes to proceed.Jun 10 2020, 3:18 PM
ngraham accepted this revision.Jun 10 2020, 3:18 PM

Oops.

This revision is now accepted and ready to land.Jun 10 2020, 3:18 PM
meven accepted this revision.Jun 10 2020, 3:30 PM
meven added inline comments.
cmake/FindTaglib.cmake
2

this is present in extra-cmake-modules/attic/modules/FindTaglib.cmake I guess you can reuse this file, instead of duplicating it here.
But I am not sure if that's possible.

cmake/FindTaglib.cmake
2

Nope, that's a legacy file that should not be used anymore.

@hschaefer still around ?

This should be good to merge. @elvisangelaccio ?

@hschaefer still around ?

I'm breathing and I'm alive.

What I need to do next?

elvisangelaccio requested changes to this revision.Jun 21 2020, 9:27 PM

Please either copy the FindTaglib.cmake from ECM master or require KF5 >= 5.72 and don't copy FindTaglib.cmake here.

ffmpegthumbnailer.cpp
50

Please use toLocal8Bit() + data() instead: https://wiki.qt.io/Technical_FAQ#How_can_I_convert_a_QString_to_char.2A_and_vice_versa.3F

Also, prefer a descriptive variable name.

This revision now requires changes to proceed.Jun 21 2020, 9:27 PM
hschaefer updated this revision to Diff 83305.Jun 22 2020, 3:25 PM
  • copied and replaced FindTaglib.cmake from KDE/extra-cmake-modules/master/find-modules/
  • using`toLocal8Bit() + data()` to get C-string of path
elvisangelaccio requested changes to this revision.Jul 5 2020, 4:26 PM
elvisangelaccio added inline comments.
CMakeLists.txt
19

Please remove.

26

This shouldn't be needed, the module will already add the taglib's include dir.

45

Please use Taglib::Taglib instead.

tests/CMakeLists.txt
23

Please use Taglib::Taglib instead.

This revision now requires changes to proceed.Jul 5 2020, 4:26 PM
hschaefer updated this revision to Diff 83329.Jul 12 2020, 7:29 AM
  • removed commented out find_package
  • replaced Taglib::Taglib
hschaefer marked 4 inline comments as done.Jul 12 2020, 7:31 AM

You uploaded a wrong version of the patch. Please do a git rebase master and make sure you only have one commit on top of origin/master.

hschaefer updated this revision to Diff 83330.Jul 13 2020, 9:03 AM

After struggling with git (as usual since git rebase master did nothing) I came up with that diff.

I did:

  • git fetch
  • git diff origin/master

I hope this diff is okay now.

hschaefer marked an inline comment as done.Jul 13 2020, 9:05 AM
meven added a comment.Jul 16 2020, 7:58 AM

After struggling with git (as usual since git rebase master did nothing) I came up with that diff.

This is expected, if you have an old master branch / the same as when you started your branch.
You can rebase only on branches that are not your parent.

I did:

  • git fetch
  • git diff origin/master

This does not edit your branch.

Once you have fetched changes on your local master, then you can git rebase master.

So run git rebase master now, that you ran git fetch. If it does nothing ensure you fetched the master branch git fetch master.

If you are working locally on your master branch, i would suggest you to checkout a new branch where you are git checkout -b diff-D23168, then reset git reset --hard master origin/master to reset your local master branch to the origin/master branch.
Then you can rebase your branch diff-D23168 on master. git checkout diff-D23168 && git rebase master.

If that's not clear I can recommend you the pro-git book reference on rebasing https://git-scm.com/book/en/v2/Git-Branching-Rebasing

hschaefer added a comment.EditedJul 17 2020, 1:09 PM

How do I switch to the right repo?

git remote -v

origin http://anongit.kde.org/ffmpegthumbs (fetch)
origin http://anongit.kde.org/ffmpegthumbs (push)


I've created a branch, changed back to master, did fetchand pull, switched back to my branch and did rebase master. Nothing happened.

I have absolutely no plan what you want me to do, except I have the wrong repo and you want me to commit somewhere else. Up to now I used the web interface to submit my patches.


git reset --hard master origin/master

fatal: Cannot do hard reset with paths.

meven added a comment.Jul 17 2020, 1:29 PM

How do I switch to the right repo?

git remote -v

origin http://anongit.kde.org/ffmpegthumbs (fetch)
origin http://anongit.kde.org/ffmpegthumbs (push)

git remote set-url origin https://invent.kde.org/multimedia/ffmpegthumbs.git

git help <command> can help you, and you can learn this from https://git-scm.com/book/en/


I've created a branch, changed back to master, did fetchand pull, switched back to my branch and did rebase master. Nothing happened.

I have absolutely no plan what you want me to do, except I have the wrong repo and you want me to commit somewhere else.
Up to now I used the web interface to submit my patches.

Didn't know that. I am not familiar with the web interface but I guess that works similarly to the arc command line utility.

By the way, while we can finish this here, the project has moved to a gitlab instance at https://invent.kde.org/multimedia/ffmpegthumbs

The simplest way then would perhaps to redo you your changes starting from current master and open a merge request in https://invent.kde.org/multimedia/ffmpegthumbs.
You might be more familiar with github/gitlab workflow.

hschaefer added a comment.EditedJul 17 2020, 2:17 PM

I added my ssh-key to gitlab, set the upstream urls:

origin git@invent.kde.org:multimedia/ffmpegthumbs.git (fetch)
origin git@invent.kde.org:multimedia/ffmpegthumbs.git (push)

Than I follwed the instructions here: https://docs.gitlab.com/ee/user/project/merge_requests/creating_merge_requests.html#new-merge-request-from-your-local-environment

git push origin diff-D23168

remote:
remote: ========================================================================
remote:
remote: You are not allowed to push code to this project.
remote:
remote: ========================================================================
remote:
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.

Really, there have been easier ways to submit a patch. Initially, it was a feature request with code provided and now I struggle months to get it into KDE. :-/

meven added a comment.Jul 18 2020, 8:51 AM

I added my ssh-key to gitlab, set the upstream urls:

origin git@invent.kde.org:multimedia/ffmpegthumbs.git (fetch)
origin git@invent.kde.org:multimedia/ffmpegthumbs.git (push)

Than I follwed the instructions here: https://docs.gitlab.com/ee/user/project/merge_requests/creating_merge_requests.html#new-merge-request-from-your-local-environment

git push origin diff-D23168

You need to fork the repository in gitlab, before opening the merge request.
Refer to our own instructions https://community.kde.org/Infrastructure/GitLab#Fork_the_project
We also have a tool https://invent.kde.org/sdk/git-lab to simplify the merge request creation/process (it is similar to arc but way easier to install, to use and to contribute to).

Really, there have been easier ways to submit a patch. Initially, it was a feature request with code provided and now I struggle months to get it into KDE. :-/

IMHO Gitlab is improving things, but since things are transitioning, this adds some friction we are seeing here.
Contributing is not a simple thing : with reviewer time varying and limited and contributors experience varying, this adds some friction. The first patch is usually the hardest.
As long as you don't have to complicate things, things are mostly smooth : here you need to rebase a patch that you opened from the web ui which phabricator does not help with.
For instance git-lab offers in Web Ui a rebase button.

Hold on a little more ;)

In D23168#675986, @meven wrote:>

You need to fork the repository in gitlab, before opening the merge request.
Refer to our own instructions https://community.kde.org/Infrastructure/GitLab#Fork_the_project
We also have a tool https://invent.kde.org/sdk/git-lab to simplify the merge request creation/process (it is similar to arc but way easier to install, to use and to contribute to).

Did a merge request.
https://invent.kde.org/multimedia/ffmpegthumbs/-/merge_requests/2

In D23168#675986, @meven wrote:>

IMHO Gitlab is improving things, but since things are transitioning, this adds some friction we are seeing here.
Contributing is not a simple thing : with reviewer time varying and limited and contributors experience varying, this adds some friction. The first patch is usually the hardest.
As long as you don't have to complicate things, things are mostly smooth : here you need to rebase a patch that you opened from the web ui which phabricator does not help with.
For instance git-lab offers in Web Ui a rebase button.

Hold on a little more ;)

I'm willing to follow all requirements, but if it gets a technical chaos and not a chaos in my contribution, I get annoyed.
I hope the merge request is okay so far. It is ONE commit.

@hschaefer Please abandon this review. Thanks :)

hschaefer abandoned this revision.Jul 27 2020, 2:12 AM