[FFmpeg-devel] [PATCH] ffprobe: Add a few common disposition cases

[Reimar Döffinger]

On 18 Sep 2012, at 20:04, Derek Buitenhuis <derek.buitenhuis at gmail.com> wrote:
> On 18/09/2012 1:53 PM, Reimar Döffinger wrote:
>>> The top two apply to audio and video streams, and the latter can only apply
>>> to video streams. It is incorrect to test all three at the same location.
>> 
>> So that leaves two cases
>> 1) it is only ever set for video streams. Then nothing changes by checking them all in on place except that the code is clearer, more obvious and can be extracted into a separate function.
> 
> I do not think it should be a separate function, and I do not think it makes it clearer than having it
> under the media type where it belongs.
> 
> I respectfully disagree.

Well, I think it belongs more to the disposition checks than a specific media type.

>> 2) somehow attached_picture gets set for an non-video steam. Why would ffprobe not printing it be a good thing?
> 
> If it gets set for anything but a video stream, then it is wrong, and a bug. "Attached Pic" on anything
> but video is hilariously stupid.

Mostly because I'd consider "attached picture" a naming error that needlessly limits what it can be used for.
What it really means "metadata that we for some reason export as stream".
And that has no real reason to be specific to video streams...
More specifically, my minor concern is that by putting this in a separate place the chance that we miss updating it if we change that name (while keeping the attached_picture for compatibility) is higher.
Anyway the patch is not worth that much discussion, ok from my side if I can't convince you.