Created attachment 246780 [details] [review]
pbutils: descriptions: Allow smart codec tag handling

Comment on attachment 246780 [details] [review]
pbutils: descriptions: Allow smart codec tag handling

That's a nice idea indeed. I only added these flags recently, that's why this function is like it is. Maybe we should just add a new function that doesn't take the tag argument? (_add_codec_to_tag_list?)

>+  /* Attempt to find tag classification */
>+  if (codec_tag == NULL) {
>+    /* Only use GST_TAG_{AUDIO|VIDEO|SUBTITLE}_CODEC for non-container formats */
>+    if (info->flags == FLAG_AUDIO)
>+      codec_tag = GST_TAG_AUDIO_CODEC;
>+    else if (info->flags == FLAG_VIDEO)
>+      codec_tag = GST_TAG_VIDEO_CODEC;
>+    else if (info->flags == FLAG_SUB)
>+      codec_tag = GST_TAG_SUBTITLE_CODEC;
>+    else
>+      codec_tag = GST_TAG_CODEC;
>+  }

I think this checking for non-container formats should be done differently here, to be more future proof in case other flags are added. In particular, I intend to add flags to signal whether formats are parsable/ES streams soon.

Oh, and for containers (and tags like id3/ape) we should use GST_TAG_CONTAINER_FORMAT instead of GST_TAG_CODEC.

Sounds like a good idea, not sure if we should add an additional function without that argument though. Is it worth it?

(In reply to comment #2)
> (From update of attachment 246780 [details] [review])
> That's a nice idea indeed. I only added these flags recently, that's why this
> function is like it is. Maybe we should just add a new function that doesn't
> take the tag argument? (_add_codec_to_tag_list?)

As Sebastian said, is it worth it ?

> 
> >+  /* Attempt to find tag classification */
> >+  if (codec_tag == NULL) {
> >+    /* Only use GST_TAG_{AUDIO|VIDEO|SUBTITLE}_CODEC for non-container formats */
> >+    if (info->flags == FLAG_AUDIO)
> >+      codec_tag = GST_TAG_AUDIO_CODEC;
> >+    else if (info->flags == FLAG_VIDEO)
> >+      codec_tag = GST_TAG_VIDEO_CODEC;
> >+    else if (info->flags == FLAG_SUB)
> >+      codec_tag = GST_TAG_SUBTITLE_CODEC;
> >+    else
> >+      codec_tag = GST_TAG_CODEC;
> >+  }
> 
> I think this checking for non-container formats should be done differently
> here, to be more future proof in case other flags are added. In particular, I
> intend to add flags to signal whether formats are parsable/ES streams soon.

It's private code ... so can be changed the moment other flags are added/updated, no ?


(In reply to comment #3)
> Oh, and for containers (and tags like id3/ape) we should use
> GST_TAG_CONTAINER_FORMAT instead of GST_TAG_CODEC.

Was wondering if there was such a tag, stupidly I was search for GST_TAG_<something>_CODEC :)

> As Sebastian said, is it worth it ?

Maybe not. Let's not then.
 
> > I think this checking for non-container formats should be done differently
> > here, to be more future proof in case other flags are added. In particular, I
> > intend to add flags to signal whether formats are parsable/ES streams soon.
> 
> It's private code ... so can be changed the moment other flags are
> added/updated, no ?

But no one will think of it when it happens, and the code isn't conceptually right - it's flags, so they should be treated as flags IMHO. Let's just do it right from the start if we can.

so just to take the audio codec example, that will require us to check that it's FLAG_AUDIO but not (any of the others).

I could have done if (flag & FLAG_AUDIO == FLAG_AUDIO) if that's what you want, but I just took the shortcut.

I think it should be:

  if (flag & (FLAG_CONTAINER | FLAG_AUDIO) == FLAG_AUDIO)

or

  if (((flag & FLAG_AUDIO)) && !((flag & FLAG_CONTAINER)))

no?

stupidities aside, I'll change it to (flag & SOME_FLAG == SOME_FLAG) then.

gah, I'm tired...

Can we just leave flag == SOME_FLAG  ?

Created attachment 246825 [details] [review]
pbutils: descriptions: Allow smart codec tag handling

We already have internally the information on what type of stream (audio,
video, container, subtitle, ...) a certain caps is.
Instead of forcing callers to specify which CODEC_TAG category a certain
caps is, use that information to make a smart choice.

Does not break previous behaviour of gst_pb_utils_add_codec_description_to_tag_list
(if tag is specified it will be used, if caps is invalid it will be rejected,
...).

Sorry but I have to agree with Tim here, just fix that and push it please ;) Flags should be used as flags

I found a much cleaner way which should satisfy every one. Patch incoming.

Created attachment 247228 [details] [review]
pbutils: descriptions: Allow smart codec tag handling

We already have internally the information on what type of stream (audio,
video, container, subtitle, ...) a certain caps is.
Instead of forcing callers to specify which CODEC_TAG category a certain
caps is, use that information to make a smart choice.

Does not break previous behaviour of gst_pb_utils_add_codec_description_to_tag_list
(if tag is specified it will be used, if caps is invalid it will be rejected,
...).

Please push.

commit a9e4750674824599dec6df0634864eec5866c764
Author: Edward Hervey <edward@collabora.com>
Date:   Fri Jun 14 07:23:40 2013 +0200

    pbutils: descriptions: Allow smart codec tag handling
    
    We already have internally the information on what type of stream (audio,
    video, container, subtitle, ...) a certain caps is.
    Instead of forcing callers to specify which CODEC_TAG category a certain
    caps is, use that information to make a smart choice.
    
    Does not break previous behaviour of gst_pb_utils_add_codec_description_to_tag_list
    (if tag is specified it will be used, if caps is invalid it will be rejected,
    ...).
    
    https://bugzilla.gnome.org/show_bug.cgi?id=702215