Audio with cover art should be opened as audio instead of video

[vibhavsinha]

Q	A
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Fixed tickets	fixes #323
Related issues/PRs	#402 (comment)
License	MIT

What's in this PR?

• Edited the $ffmpeg->open() method to send Video if and only if at least one video stream is not marked as attatched_pic.
• A sample audio file with cover which is created by adding a cover image to the already existing audio.mp3 using eye-d3
• Functional tests to confirm the reported issue and verify the fix.

Why?

Fixes #323
FFMmpeg::open() has 'media type' bug with MP3s (image metadata)

Fffmpeg source uses this function to detect if a video stream is actually a valid video stream or just a cover art.
https://github.com/FFmpeg/FFmpeg/blob/release/4.2/libavformat/movenc.c#L149-L154

I did not use the patch in #402 (comment) because the codec_type can be either png or jpeg. If the codec_type is jpeg, that stream can be either a cover art or just a MotionJPEG video stream. Hence, it was not the right way to detect this.

BC Breaks/Deprecations

No.
There is a small chance that some code depends on this behavior to detect audio with cover images.

To Do

• Create tests to confirm the issue
• Fix till the new tests pass
• Run full test suite after making the change

[jens1o]

Thank you for preparing this fix. However, I still think this might be a breaking change. @alexander-schranz how would you like to proceed here? File a new major version or don't you think it is a breaking change?

[jens1o]

minor nit: You can add a break; after this to not iterate unnecessarily.

[vibhavsinha]

thanks. done. vibhavsinha@7b57207

[fpolli]

There is a missing closing brace in this fix. It opens 3 and only closes 2

[vibhavsinha]

The code has been changed since the last comment here. I am sure it is balanced now. The reason for opening 3 and closing 2 is because the subtracted chunk also had one opening bracket. Hence, the added chunk should also have one extra opening brace.

[alexander-schranz]

I find this a little bit strange, should we not use instead a mime_type check on pathfile to check if its a video or audio file. I could think that there are videos without a attached_pic disposition so I would not depend on that.
Also it feels for me the false position to fix it as $streams->videos() should not return none video file atleast I would not expect that.

@romainneutron do you remember why this was done before over count($streams->videos()) instead of checking for mimetype?

[jens1o]

If it is possible to reply for @romainneutron yet due to the current circumstances, I'd be happy to bring this a little bit forward. :)

[vibhavsinha]

@alexander-schranz It is a valid idea to use the mime_type for identifying the audio and video formats. But I can guess why it might not have been chosen for this project.

There are way too many formats for audio and video both. And I am sure there are some which can be used to hold only one stream of either type. Keeping a list of mime types will limit us to a subset of formats supported, and it will be different from ffmpeg behavior.

AFAIK ffmpeg does not differentiate between audio and video files. It only uses the mime_type to parse the container structure. After parsing container, it works only with stream information.

[vibhavsinha]

• I have rebased this branch with the latest of master because there were way too many changes since then.
• Thanks to the improved test cases, I also found a bug in my code when reading $videoStream->get('disposition'). It is fixed now. I have verified that test case passed in my fork.
• Thanks for @fpolli for mentioning and reminding me of this PR. I do not do much PHP now, so had an opportunity to re-learn phpunit 😄. There was a question about mismatched braces that I did not understand. It somehow looked okay to me. Nonetheless, after the rebase and test cases, it should be fine.

[sgloe]

I would appreciate this fix as well.