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 792260 - msdk: decode: Add mpeg2 decoder
msdk: decode: Add mpeg2 decoder
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other Linux
: Normal enhancement
: 1.13.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks: 789886
 
 
Reported: 2018-01-06 01:34 UTC by sreerenj
Modified: 2018-01-11 02:24 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
msdk: Add mpeg2 decdoer (8.28 KB, patch)
2018-01-06 01:37 UTC, sreerenj
none Details | Review
msdk: Add mpeg2 decoder (8.30 KB, patch)
2018-01-08 19:51 UTC, sreerenj
committed Details | Review

Description sreerenj 2018-01-06 01:34:40 UTC
Enable mpeg2 decoder in MediaSDK plugin.

Patch is pretty straightforward, just the matter of enabling.
Comment 1 sreerenj 2018-01-06 01:37:30 UTC
Created attachment 366403 [details] [review]
msdk: Add mpeg2 decdoer
Comment 2 Sebastian Dröge (slomo) 2018-01-06 11:03:46 UTC
Review of attachment 366403 [details] [review]:

::: sys/msdk/gstmsdkmpeg2dec.c
@@ +48,3 @@
+        "framerate = (fraction) [0/1, MAX], "
+        "width = (int) [ 1, MAX ], height = (int) [ 1, MAX ], "
+        "mpegversion = 2")

This probably also handles mpegversion=1 or not?
And you're missing systemstream=false here
Comment 3 sreerenj 2018-01-08 19:51:17 UTC
(In reply to Sebastian Dröge (slomo) from comment #2)
> Review of attachment 366403 [details] [review] [review]:
> 
> ::: sys/msdk/gstmsdkmpeg2dec.c
> @@ +48,3 @@
> +        "framerate = (fraction) [0/1, MAX], "
> +        "width = (int) [ 1, MAX ], height = (int) [ 1, MAX ], "
> +        "mpegversion = 2")
> 
> This probably also handles mpegversion=1 or not?

NO, Only version 2.

> And you're missing systemstream=false here

My fault! Thanks for the review
Comment 4 sreerenj 2018-01-08 19:51:49 UTC
Created attachment 366510 [details] [review]
msdk: Add mpeg2 decoder
Comment 5 Sebastian Dröge (slomo) 2018-01-08 20:26:52 UTC
(In reply to sreerenj from comment #3)
> (In reply to Sebastian Dröge (slomo) from comment #2)
> > Review of attachment 366403 [details] [review] [review] [review]:
> > 
> > ::: sys/msdk/gstmsdkmpeg2dec.c
> > @@ +48,3 @@
> > +        "framerate = (fraction) [0/1, MAX], "
> > +        "width = (int) [ 1, MAX ], height = (int) [ 1, MAX ], "
> > +        "mpegversion = 2")
> > 
> > This probably also handles mpegversion=1 or not?
> 
> NO, Only version 2.

That would be quite surprising. Did you try? MPEG1 is approximately a subset of MPEG2.
Comment 6 sreerenj 2018-01-08 20:53:04 UTC
(In reply to Sebastian Dröge (slomo) from comment #5)
> (In reply to sreerenj from comment #3)
> > (In reply to Sebastian Dröge (slomo) from comment #2)
> > > Review of attachment 366403 [details] [review] [review] [review] [review]:
> > > 
> > > ::: sys/msdk/gstmsdkmpeg2dec.c
> > > @@ +48,3 @@
> > > +        "framerate = (fraction) [0/1, MAX], "
> > > +        "width = (int) [ 1, MAX ], height = (int) [ 1, MAX ], "
> > > +        "mpegversion = 2")
> > > 
> > > This probably also handles mpegversion=1 or not?
> > 
> > NO, Only version 2.
> 
> That would be quite surprising. Did you try? MPEG1 is approximately a subset
> of MPEG2.

I haven't tried. But at least MSDK spec only advertising MFX_CODEC_MPEG2
Is it the exact subset? I don't think so, but may be I am mistaken. I don't remember exactly, but there were mpeg1 streams which weren't working correctly with gstreamer-vaapi.
Comment 7 sreerenj 2018-01-08 20:58:10 UTC
(In reply to sreerenj from comment #6)
> (In reply to Sebastian Dröge (slomo) from comment #5)
> > (In reply to sreerenj from comment #3)
> > > (In reply to Sebastian Dröge (slomo) from comment #2)
> > > > Review of attachment 366403 [details] [review] [review] [review] [review] [review]:
> > > > 
> > > > ::: sys/msdk/gstmsdkmpeg2dec.c
> > > > @@ +48,3 @@
> > > > +        "framerate = (fraction) [0/1, MAX], "
> > > > +        "width = (int) [ 1, MAX ], height = (int) [ 1, MAX ], "
> > > > +        "mpegversion = 2")
> > > > 
> > > > This probably also handles mpegversion=1 or not?
> > > 
> > > NO, Only version 2.
> > 
> > That would be quite surprising. Did you try? MPEG1 is approximately a subset
> > of MPEG2.
> 
> I haven't tried. But at least MSDK spec only advertising MFX_CODEC_MPEG2
> Is it the exact subset? I don't think so, but may be I am mistaken. I don't
> remember exactly, but there were mpeg1 streams which weren't working
> correctly with gstreamer-vaapi.

Maybe it was the parser issue (wrongly advertising mpeg version , or something like that). According to specification, mpeg2 decoder should be able to decode mpeg1 streams too. So shall we just enable it? :)
Comment 8 sreerenj 2018-01-08 21:42:11 UTC
I did some testing, mpeg1 stream doesn't seem to be decodable with MediaSDK (at least with MediaServerStudioEssentials2017R3).
Comment 9 Sebastian Dröge (slomo) 2018-01-09 08:29:01 UTC
Comment on attachment 366510 [details] [review]
msdk: Add mpeg2 decoder

Ok, go for it then.
Comment 10 sreerenj 2018-01-10 18:41:02 UTC
Review of attachment 366510 [details] [review]:

Pushed: commit 7536d1209573c357cdf8b443aa8ad155cf74cf3c
Comment 11 Víctor Manuel Jáquez Leal 2018-01-10 20:14:08 UTC
Review of attachment 366510 [details] [review]:

::: sys/msdk/gstmsdkmpeg2dec.c
@@ +40,3 @@
+
+GST_DEBUG_CATEGORY_EXTERN (gst_msdkmpeg2dec_debug);
+#define GST_CAT_DEFAULT gst_msdkmpeg2dec_debug

I guess you forgot to add, for sake of completeness, the GST_DEBUG_CATEGORY_INIT of this category, though I'm not sure if it is used at all (as other debugging categories for decoders)
Comment 12 sreerenj 2018-01-11 02:24:50 UTC
(In reply to Víctor Manuel Jáquez Leal from comment #11)
> Review of attachment 366510 [details] [review] [review]:
> 
> ::: sys/msdk/gstmsdkmpeg2dec.c
> @@ +40,3 @@
> +
> +GST_DEBUG_CATEGORY_EXTERN (gst_msdkmpeg2dec_debug);
> +#define GST_CAT_DEFAULT gst_msdkmpeg2dec_debug
> 
> I guess you forgot to add, for sake of completeness, the
> GST_DEBUG_CATEGORY_INIT of this category, though I'm not sure if it is used
> at all (as other debugging categories for decoders)

Fixed, Thank you :)