Created attachment 176043 [details] [review]
add function to map (container) caps to filename extension

Two small points for review:

1.) the caps check uses gst_caps_is_subset(), gst_caps_is_equal would be quite strict. Any opinions?

2.) the function would return NULL, if the first matching typefinder has no extensions. Is the assumtion that there should be no less prioritized typefinder in the list,k that has extensions okay?

Created attachment 176050 [details] [review]
add function to map (container) caps to filename extension

Remove some gst-indent noise from the patch.

* You say that the caps should be fixed but I think you mean simple (i.e. one structure) caps, like in your testcase. You should also add a g_return_val_if_fail(GST_CAPS_IS_SIMPLE(caps), NULL) at the beginning of the function
* If you check for subset from both directions you can simply use gst_caps_can_intersect(). It's also much faster. IMHO checking for an intersection is good enough here
* IMHO there should be no lower-ranked typefinder that has extensions while a high-ranked typefinder does not have any. That would be a bug in the high-ranked typefinder.
* Your testcase exposes a small problem already. Usually MPEG 1 Layer 2 will use mp2 as extension, not mp3. (and use fail_unless_equals_string())

Have you updated core? There was a bug in the ordering of the extensions in serialisation and deserialisation of the registry. The other comments make sense. Will update the patch and add more tests.

Forgot to mention, bilboed disliked the idea of reusing/misusing typefinders here. I agree in principle, but don't think it is too bad, as it is an implementation detail.

Created attachment 176326 [details] [review]
add function to map (container) caps to filename extension

Take the updates into account. I found cases where the extensions in the typefinder don't have the most common one first. Do we want to add such a guideline?

From
"gstreamer/gst-plugins-base/gst/typefind/gsttypefindfunctions.c::plugin_init()"
it is e.g. ogg_exts that has "anx" instead of "ogg" first. For some formats like mpeg_*_exts or rm_exts its also a bit ambiguous.

Not that I'm opposed to this, but I'm wondering whether it wouldn't be better if this kind of info was part of the encoding profile stuff instead.

There are things we'll never get right with the API proposed here - for example: it should be 'ogv' for ogg video and not just 'ogg'.

Or things like 'mp4', '3gp', 'mov' etc.

We could add a 'extension' field to the container profiles...

... but it would stil be nice if there was a default choice done in order not to force users to always write that.

(In reply to comment #7)
> Not that I'm opposed to this, but I'm wondering whether it wouldn't be better
> if this kind of info was part of the encoding profile stuff instead.
> 
> There are things we'll never get right with the API proposed here - for
> example: it should be 'ogv' for ogg video and not just 'ogg'.

  Not much we can do about that use-case (and the asf+wma => .wma cases). Those would indeed have to be hardcoded.

> 
> Or things like 'mp4', '3gp', 'mov' etc.

  That's another reason why re-using the typefinders instead of (extending) pbutils/description is a bad idea.

  We could add an additional field in the descriptions internal structure to contain recommended extensions.
  In addition, we could have support for the variants of quicktime:
  variant=apple => .mov
  variant=3gpp => .3gp
  variant=iso => .mp4

Let's try to find a solution that doesn't involve re-using the info from the typefinders then.

If we can't get common use cases right with this API/implementation, maybe it's best not to add it after all.

I am happy with a better solution. At least we seem to agree that it would be useful functionality.

1.) As an alternative we could attach it to the FormatInfo in pbutils/descriptions.c. We could have a fixed extension or a callback that resolves it according to the content.

The api would then be 
const char *gst_pb_utils_suggest_file_name_extension(type, flags)
flags would be: has_video, has_audio

2.) or we just have three extension fields attached to FormatInfo:
audio_and_video
audio_only
video_only

and select the appropriate one based on the flags.

Where do you get the caps from in 'format-independent encoding and transcoding apps' ?

I wonder whether it would make sense to make it a full GstEncodingProfile then right away.

Only problem I see is with things like application/x-id3 or so, where the file extension is determined by the actual audio format and not the "container/tag" (but if we just pass caps we don't have any info about that).

Another "complication": audio-only Ogg -> .oga, but if it's Speex-in-Ogg -> .spx

Lets make it part of the encoding profiles then.

commit a3c6d0da911f24aeca76b1c8fe9e5be5490cd9a0
Author: Tim-Philipp Müller <tim@centricular.net>
Date:   Sun Dec 23 15:51:51 2012 +0000

    encoding-profile: add special-casing for asf/wmv/wma file extensions
    
    https://bugzilla.gnome.org/show_bug.cgi?id=636753

commit 42f971c5eb7998faa742fb7479834c6445db730e
Author: Tim-Philipp Müller <tim@centricular.net>
Date:   Sun Dec 23 15:26:59 2012 +0000

    encoding-profile: add gst_encoding_profile_get_file_extension()
    
    API: gst_encoding_profile_get_file_extension()
    
    https://bugzilla.gnome.org/show_bug.cgi?id=636753