After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 702215 - pbutils: descriptions: Allow smart codec tag handling
pbutils: descriptions: Allow smart codec tag handling
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-base
unspecified
Other All
: Normal enhancement
: 1.1.2
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks: 702216
 
 
Reported: 2013-06-14 05:26 UTC by Edward Hervey
Modified: 2013-06-28 05:11 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
pbutils: descriptions: Allow smart codec tag handling (6.26 KB, patch)
2013-06-14 05:26 UTC, Edward Hervey
reviewed Details | Review
pbutils: descriptions: Allow smart codec tag handling (6.83 KB, patch)
2013-06-14 16:38 UTC, Edward Hervey
none Details | Review
pbutils: descriptions: Allow smart codec tag handling (6.64 KB, patch)
2013-06-19 05:55 UTC, Edward Hervey
accepted-commit_now Details | Review

Description Edward Hervey 2013-06-14 05:26:49 UTC
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,
...).
Comment 1 Edward Hervey 2013-06-14 05:26:52 UTC
Created attachment 246780 [details] [review]
pbutils: descriptions: Allow smart codec tag handling
Comment 2 Tim-Philipp Müller 2013-06-14 09:10:24 UTC
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.
Comment 3 Tim-Philipp Müller 2013-06-14 09:11:45 UTC
Oh, and for containers (and tags like id3/ape) we should use GST_TAG_CONTAINER_FORMAT instead of GST_TAG_CODEC.
Comment 4 Sebastian Dröge (slomo) 2013-06-14 12:31:47 UTC
Sounds like a good idea, not sure if we should add an additional function without that argument though. Is it worth it?
Comment 5 Edward Hervey 2013-06-14 13:26:18 UTC
(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 :)
Comment 6 Tim-Philipp Müller 2013-06-14 13:32:55 UTC
> 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.
Comment 7 Edward Hervey 2013-06-14 14:11:37 UTC
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.
Comment 8 Tim-Philipp Müller 2013-06-14 14:16:34 UTC
I think it should be:

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

or

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

no?
Comment 9 Edward Hervey 2013-06-14 16:29:00 UTC
stupidities aside, I'll change it to (flag & SOME_FLAG == SOME_FLAG) then.
Comment 10 Edward Hervey 2013-06-14 16:31:14 UTC
gah, I'm tired...

Can we just leave flag == SOME_FLAG  ?
Comment 11 Edward Hervey 2013-06-14 16:38:21 UTC
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,
...).
Comment 12 Sebastian Dröge (slomo) 2013-06-18 09:16:21 UTC
Sorry but I have to agree with Tim here, just fix that and push it please ;) Flags should be used as flags
Comment 13 Edward Hervey 2013-06-19 05:51:47 UTC
I found a much cleaner way which should satisfy every one. Patch incoming.
Comment 14 Edward Hervey 2013-06-19 05:55:09 UTC
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,
...).
Comment 15 Tim-Philipp Müller 2013-06-27 22:46:58 UTC
Please push.
Comment 16 Edward Hervey 2013-06-28 05:11:37 UTC
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