Skip to content
This repository has been archived by the owner on Jan 12, 2019. It is now read-only.

Refactor h264stream #272

Closed
wants to merge 13 commits into from
Closed

Refactor h264stream #272

wants to merge 13 commits into from

Conversation

gkatsev
Copy link
Member

@gkatsev gkatsev commented May 18, 2015

No description provided.

@gkatsev gkatsev force-pushed the refactor-h264frame branch from 6490d3a to 047a6d7 Compare May 18, 2015 21:06
The Random Access Indicator tells us whether something is a keyframe.
Set the stream's frame's keyFrame property to true if the Random Access
Indicator is set.
adaptation_field_length,
afftemp,
discontinuity_indicator,
random_access_indicator,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we only really care about the random_access_indicator currently, so, I could remove the others if needed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this code is very inconsistent about variable naming but we should stick with camel case.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I literally copy/pasted this from the spec :), but yes, sticking with camel case is fine.

@gkatsev
Copy link
Member Author

gkatsev commented May 20, 2015

Looking into build failure.

@@ -0,0 +1,213 @@
(function() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What changed in here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved the H264ExtraData object into its own file instead of having it live inside of h264-stream.js

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the future, please try to split refactor PRs from functionality-changing PRs so it's clearer what needs to be reviewed.

h264Frame = new FlvTag(FlvTag.VIDEO_TAG);
h264Frame.pts = next_pts;
h264Frame.dts = next_dts;
if (this._nextFrameKeyFrame) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when creating a new h264Frame, check to see if it'll be a keyframe.

} else {
h264Frame.length -= 1; // 00 : 00 01
}
H264Stream.prototype.setNextFrameKeyFrame = function() {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

where the function is defined.

@dmlap
Copy link
Member

dmlap commented May 21, 2015

LGTM

@dmlap dmlap closed this in 63c045e May 22, 2015
@dmlap dmlap deleted the refactor-h264frame branch May 22, 2015 18:06
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants