Adddate: 2010-01-15 11:08:46
Would it make more sense to have packetizedStream=TRUE instead of
byteStream=FALSE? I would assume most codecs support byteStream, so it's not
clear exactly what behavior byteStream=FALSE enables.
It looks like the primary motivation for having #if 0 code is that
BufferGfx_getFrameType() isn't supported by the versions of DMAI we currently
use. Is there any way to work-around this missing DMAI feature in our current
In general, I don't like checking-in commented-out code (#if 0) to the trunk.
If there isn't a cleaner way to turn this on/off, IMHO it is better to maintain
this code as a patch. It looks like checking in this patch as-is would add a
"byteStream=FALSE" property that does nothing.
Regarding the code itself, I think the code you added that creates the codecData
information is extremely useful, and it would be really good to get this in -- I
hope there is a feasible work-around for the DMAI issue.
In the encode thread, I noticed that you added a significant portion of code to
the main thread loop. I think we need to try hard to keep this function as
tight as possible for readability purposes (keep the focus on the main algorithm
of looping and encoding). Is there any way we can break-up your additional code
Adddate: 2010-01-15 11:34:49
Thanks for review.
gst-ffmeg video encoder (i.e x264enc) has a property named
"byte-stream" and I tried to keep the same name. So that the
gst folks know what exactly this property does in TI Videnc.
Only workaround, i can think of is implementing this function inside our
elements itself. But implementing BufferGfx_getFrameType will requires including
DMAI "private" header file. I personally don't like including
"private" header file (as they may change from
release-to-release). Again open for the idea. If you feel we can use it then let
No by default encoder is generating byte-stream and the code in #if 0 is doing
some addition processing on generated byte-stream buffer. This mainly helps
I agree with your point on keeping the function as tight as possible and may be
i can write "gst_tividdec2_parse_and_push(....)" which will
add header (if needed) and push the buffer to downstream. But we don't want to
separate these two steps in two different fxns for the optimization reasons.
Adddate: 2010-01-15 12:31:15
Given that there is already precedent for byteStream, I guess I'm ok with it
then in the interests of consistency.
Regarding the DMAI work-around, is there any way we could use
Vdec2_getVisaHandle() instead of including private headers?
Still not sure if I'm clear on the #if 0 code. From what you say, it sounds
like you disagree with my statement and are saying that byteStream=FALSE still
enables packetized streaming even with the code commented out, and that the #if
0 code is just an optimization for byteStreams? In this case, should it be a
separate patch? I still think I'm missing something here.
Adddate: 2010-01-15 13:42:53
I don't think Vdec2_getVisaHandle will help us on this case. Only other possible
soln is to parse the stream header and check the frame type instead of
BufferGfx_getFrameType API. Not sure why we should spend anytime on this when it
can be easily returned from the DMAI. I'm thinking that folks who are interested
can add this support in DMAI itself or wait for next DMAI release and then we
will uncomment the code.
Your understanding is correct, the code in #if 0 is additional optimization for
byte-stream to help streaming cases. Not sure if we should make this as separate
patch. I'm considering #if 0 as comment in source file - which will help
developer to tweak the file and enable this code after they make DMAI changes.
Adddate: 2010-01-18 10:06:10
Thanks for your review comment, i removed #if 0 part from the patch and added
parse_and_push function like explained above.
Please let me know if u have any further comments on this.
Adddate: 2010-01-18 11:45:42
One nit -- on lines 1554 and 1732 of your modified tividenc1.c, you introduced
some unneeded white space. Please remove before check-in (review not
Also, do you think the parse_and_push functions should call a generic
"h264_parse_and_push", that takes codecName, byteStream,
codec_data, and outBuf as parameters? Code that does masking and shifting is
pretty delicate, and IMHO we should try to share it. I know it adds another
function call, but I think we're still ok. What do you think?
Otherwise, looks great!
Adddate: 2010-01-18 12:41:23
For the record, I guess I'm ok without having a separate h264_parse_and_push.
It may be more trouble than it's worth. Overall, we do need to try harder at
code sharing though -- hopefully the 2.0 changes will help here.
Adddate: 2010-01-18 14:15:11
okay thanks don.