Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix for issue #1277 #1279

Merged
merged 8 commits into from
Nov 19, 2018
Merged

Fix for issue #1277 #1279

merged 8 commits into from
Nov 19, 2018

Conversation

nicolasbeauvais
Copy link
Contributor

This PR make two changes:

Minor:

The performConversion method only perform manipulations has the conversion is made in the performConversions method on line 94.
So it makes more sense to rename it performManipulationsinstead. It will also reduce confusion between performConversions and performConversion.

Fix for issue #1277:

Problem:

Creating a custom generator that returns a file that is not an image (in that case a .m3u8 file) fail because manipulations are being automatically run on that file which is not compatible.

Proposed fix:

The end user needs to disable the default manipulations for the specific conversion like:

$this->addMediaConversion('transcode')
            ->removeManipulation('format')
            ->removeManipulation('optimize')
            ->performOnCollections('file');

// We could also add a base method for this:

$this->addMediaConversion('transcode')
            ->removeAllManipulations()
            ->performOnCollections('file');

And in the core, we check if the running conversion has manipulation to be triggered, if not we skip this step and the converted file stay untouched.

I haven't dive in the core code for a long time so I might be missing something here, anyway do not hesitate to indicate any change that could be made to improve this fix.

@pedrofurtado
Copy link

@nicolasbeauvais The CI failed because of some rules of coding standard 👍

@pedrofurtado
Copy link

Ping for notice: @freekmurze

src/FileManipulator.php Outdated Show resolved Hide resolved
@freekmurze
Copy link
Member

@nicolasbeauvais Thanks for this! 👍 Could you add a test to make sure that these changes work?

@nicolasbeauvais
Copy link
Contributor Author

nicolasbeauvais commented Nov 12, 2018

WIP:

  • Refactored the fix using an early return in the performManipulations method.
  • Created a withoutManipulations helper function for conversions.
  • Added some missing return types in Conversion.php.
  • Add tests for the fix.
  • Add tests for the withoutManipulations method.
  • Update the documentation to reflect this changes

@nicolasbeauvais
Copy link
Contributor Author

I should be done with this, of course, feel free to suggest any improvements.

I will update the documentation once this gets merged!

@freekmurze freekmurze merged commit 089ee07 into spatie:master Nov 19, 2018
@freekmurze
Copy link
Member

Thank you so much for your work on this 👍

@pedrofurtado
Copy link

@freekmurze Where is versioned the docs of the package? It's great to update it to include a section explaining this feature. I can help creating a PR for this 👍

@freekmurze
Copy link
Member

@pedrofurtado https://docs.spatie.be/laravel-medialibrary/v7/introduction
There's an edit button on every page.

@chelout
Copy link

chelout commented May 18, 2020

@nicolasbeauvais @pedrofurtado could you please add an example who to work with this feature?

@sp4r74cus
Copy link

hi @pedrofurtado, can you confirm your hls conversion works with this update? i tried to make a similar conversion (mp4 -> hls) using your code as a start (#1277)

i'm facing 2 issues:

  1. the conversion file is saved as video-{conversion-name}.mp4 and not .m3u8 (the file name i return)
  2. all the .m3u8 & .ts generated in the tmp directory are not moved to the conversions folder

Thanks

@jayenne
Copy link

jayenne commented Nov 6, 2021

did the two issues above regarding conversio nfile name and moving t ocorrect dir get resolved?

  • would you be able to share a howto for converting video/audio media to .m3u8 & .ts please?
  • or point us to a guide/resource please?

@joserick
Copy link

Hi all.

the conversion file is saved as video-{conversion-name}.mp4 and not .m3u8 (the file name i return)

I had the first problem @jayenne , I solved it in the following way:

$this->addMediaCollection('audio')->singleFile()
    ->registerMediaConversions(function (Media $media) {
        $this->addMediaConversion('ogg')
            //->withoutManipulations() #Removed for redundancy in this case.
            ->setManipulations(function (Manipulations &$manipulations){
                $manipulations = new class extends Manipulations{
                    const FORMAT_OGG = 'ogg';

                    public function isEmpty(): bool
                    {
                        return true;
                    }
                };

                $manipulations->format('ogg');
            })->nonQueued();
    });
MyModel::first()->addMedia('path/to/file.wav')->toMediaCollection('audio');
MyModel::first()->getFirstMediaUrl('audio', 'ogg');
// http://localhost/storage/#/conversions/file-ogg.ogg <- Already change the extension

I hope it will help someone in the future :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants