GNOME Bugzilla – Bug 743948
vc1parser: re-enable BDU parsing for bitstream
Last modified: 2018-11-03 13:30:10 UTC
Currently the parser is not able to parse bitstream like in this pipeline: gst-launch-1.0 filesrc location=mytestfile.vc1 ! vc1parse ! .. Since the parser bails out with an error message. This patch proposes a way re-enable it.
Created attachment 296054 [details] [review] vc1parser: re-enable BDU parsing for bitstream
Review of attachment 296054 [details] [review]: Moreover, it seems that the code to detect BDU stream format is not enough as startcode can be present in sequence-layer-bdu-* stream formats, and also iirc in ASF format. So I think enable this code as if will broke other formats detection if done first. ::: gst/videoparsers/gstvc1parse.c @@ +645,3 @@ GST_DEBUG_OBJECT (vc1parse, "Found BDU startcode"); + vc1parse->input_stream_format = vc1parse->output_stream_format = + VC1_STREAM_FORMAT_BDU_FRAME; I think output_stream_format will be set at negotitation time, so we should not set it here.
What container format / decoder combinations has this been tested with? The code was originally disabled because it was causing problems, so we should test thorougly with a multitude of demuxers + decoders (with parser in between) before re-enabling that code.
(In reply to comment #3) > What container format / decoder combinations has this been tested with? The > code was originally disabled because it was causing problems, so we should test > thorougly with a multitude of demuxers + decoders (with parser in between) > before re-enabling that code. So far I'm only using the files attached in these bugs: bug 719751, bug 719752, bug 719753 and bug 719743 Definitely I want to test it with more patterns.
(In reply to comment #2) > Review of attachment 296054 [details] [review]: > > Moreover, it seems that the code to detect BDU stream format is not enough as > startcode can be present in sequence-layer-bdu-* stream formats, and also iirc > in ASF format. > So I think enable this code as if will broke other formats detection if done > first. > > ::: gst/videoparsers/gstvc1parse.c > @@ +645,3 @@ > GST_DEBUG_OBJECT (vc1parse, "Found BDU startcode"); > + vc1parse->input_stream_format = vc1parse->output_stream_format = > + VC1_STREAM_FORMAT_BDU_FRAME; > > I think output_stream_format will be set at negotitation time, so we should not > set it here. It is set in the negotiation time, but if there's nothing in the caps, output_stream_format keeps its original value of VC1_STREAM_FORMAT_BDU, and the parsing stops because the element assume a wrong conversion.
Created attachment 297628 [details] [review] vc1parser: re-enable BDU parsing for bitstream Currently the parser is not able to parse bitstream like in this pipeline: gst-launch-1.0 filesrc location=mytestfile.vc1 ! vc1parse ! .. Since the parser bails out with an error message. This patch proposes a way re-enable it using the function gst_vc1_identify_next_bdu() early. If a complete sequence header BDU is found, the stream format is assumed as bdu-frame.
ping? BTW, I have tested with h264 streams and I haven't found false positives :)
I think we have a misunderstanding here. You are interested in making 'raw' vc1 ES streams work. That's fine of course, but that is not my concern. My concern is making sure that VC-1 wrapped in major container formats continues to work fine, so testing .vc1 files that are not wrapped in containers does not really address my comment #3. > BTW, I have tested with h264 streams and I haven't found false positives :) This is in response to my comments in the other bug about the typefinder patch?
(In reply to Tim-Philipp Müller from comment #8) > I think we have a misunderstanding here. You are interested in making 'raw' > vc1 ES streams work. That's fine of course, but that is not my concern. My > concern is making sure that VC-1 wrapped in major container formats > continues to work fine, so testing .vc1 files that are not wrapped in > containers does not really address my comment #3. I guess I get it now. But I'm not sure how to proceed here. Do you mean I should get a lot of samples with different containers and verify them? In that case, what would worry me is that I don't have access to other vc1 decoders than vaapi and gst-libav, to verify them too. > > > BTW, I have tested with h264 streams and I haven't found false positives :) > > This is in response to my comments in the other bug about the typefinder > patch? Yes. I got confused mixing up things.
Yeah, doesn't have to be "a lot", just one or three for each type of container (asf/wmv, mkv, quicktime, mpeg-ts?), and then test those with your vc1parse changes + gst-libav and vaapi. That should be enough.
We tested several bitstream (ASF,MKV,QUICKTIME,MPEG-TS), they are all worked with the patch "re-enable BDU parsing for bitstream".
Hi, (In reply to Allen Zhang from comment #11) > We tested several bitstream (ASF,MKV,QUICKTIME,MPEG-TS), they are all worked > with the patch "re-enable BDU parsing for bitstream". Can you list me those test files? :) I would like to test them too.
-- GitLab Migration Automatic Message -- This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gst-plugins-bad/issues/206.