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 737411 - videoparser: comment out unused custom baseparse flag (with duplicate value)
videoparser: comment out unused custom baseparse flag (with duplicate value)
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
unspecified
Other Linux
: Normal minor
: 1.5.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2014-09-26 09:36 UTC by Vineeth
Modified: 2014-10-06 12:07 UTC
See Also:
GNOME target: ---
GNOME version: ---

Attachments
move flag to base parse (2.75 KB, patch)
2014-09-26 09:38 UTC, Vineeth 		none Details | Review
gstbaseparse.h (13.69 KB, patch)
2014-09-26 09:41 UTC, Vineeth 		none Details | Review
proposed patch (1.71 KB, patch)
2014-10-06 03:41 UTC, Vineeth 		needs-work Details | Review
Update the patch as per review comments (1.86 KB, patch)
2014-10-06 08:10 UTC, Vineeth 		committed Details | Review

Description Vineeth 2014-09-26 09:36:49 UTC 
GST_BASE_PARSE_FRAME_FLAG_PARSING, was being defined locally in videoparsers with 0x10000, but this value is already being used by 
GST_BASE_PARSE_FRAME_FLAG_QUEUE        = (1 << 4)
in gstbaseparse.h

Hence moving the flag to gstbaseparse.h to maintain consistency.
Please review
Comment 1 Vineeth 2014-09-26 09:38:56 UTC 
Created attachment 287136 [details] [review]
move flag to base parse
Comment 2 Vineeth 2014-09-26 09:41:21 UTC 
Created attachment 287137 [details] [review]
gstbaseparse.h
Comment 3 Tim-Philipp Müller 2014-09-26 09:43:52 UTC 
The FLAG_PARSING doesn't even seem to be used in videoparsers, so why not just remove it there?
Comment 4 Vineeth 2014-09-26 09:53:45 UTC 
yes even i saw it was not being used..
but in gstvc1parse.h

it is mentioned 
  /* Metadata about the currently parsed frame, only
   * valid if the GstBaseParseFrame has the
   * GST_BASE_PARSE_FRAME_FLAG_PARSING flag */
  GstVC1StartCode startcode;

and the startcode related implementation is under FIXME
FIXME: disable BDU check for now as BDU parsing needs more work.


so i thought it might be needed if someone decides to implement the FIXME.

Or if we remove the flag, then probably we should remove the code related to VC1StartCode as well?
Comment 5 Tim-Philipp Müller 2014-09-26 10:08:27 UTC 
Ok, so comment it out and increase the value perhaps. It seems like a non-issue at the moment :)
Comment 6 Vineeth 2014-09-26 10:16:51 UTC 
comment it out and keep the FIXME as it is?

yeah it is minor issue :)
Comment 7 Vineeth 2014-10-06 03:41:12 UTC 
Created attachment 287795 [details] [review]
proposed patch

Commenting out the unused flag.
Comment 8 Tim-Philipp Müller 2014-10-06 08:02:37 UTC 
Comment on attachment 287795 [details] [review]
proposed patch

Please use C-style /* comments */ instead of C++-style // comments.
Comment 9 Vineeth 2014-10-06 08:10:14 UTC 
Created attachment 287815 [details] [review]
Update the patch as per review comments
Comment 10 Luis de Bethencourt 2014-10-06 11:04:37 UTC 
https://bug737411.bugzilla-attachments.gnome.org/attachment.cgi?id=287137

Looks like it just changing the coding style but not changing the code itself.
Comment 11 Luis de Bethencourt 2014-10-06 11:42:38 UTC 
Comment on attachment 287815 [details] [review]
Update the patch as per review comments

Merged this patch.
Comment 12 Luis de Bethencourt 2014-10-06 11:54:15 UTC 
Ignored the style patches and merged the good changes.