Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Print video rotation #1400

Closed
wants to merge 6 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@Piasy
Copy link
Contributor

Piasy commented Oct 9, 2018

No description provided.

@lminiero

This comment has been minimized.

Copy link
Member

lminiero commented Oct 9, 2018

Extensions don't have a fixed ID, they're negotiated. The fact that Chrome hardcodes some values, doesn't mean others will use the same as well. As such, I'm afraid I can't merge this.

@Piasy

This comment has been minimized.

Copy link
Contributor Author

Piasy commented Oct 9, 2018

Could we add an option to specify CVO ID?

@lminiero

This comment has been minimized.

Copy link
Member

lminiero commented Oct 9, 2018

That might work, yes. Anyway, not sure why you've added the code to the h264 file: extensions are the same for all video codecs, and so that can be in the generic parser instead.

@Piasy

This comment has been minimized.

Copy link
Contributor Author

Piasy commented Oct 9, 2018

Okey, I'll add an option for that, and move it out from h264 file :)

Piasy added some commits Oct 9, 2018

check video rotation from RTP header extension, and print it to console
With that information, we could rotate the video when combine audio and video data into a single video file using ffmpeg.

For example, rotate the video 90 degree clockwise:

ffmpeg -i audio.opus -i video.mp4 -c:a opus \
     -vf "transpose=1" \
    -strict experimental merged_output.mp4
print RTP header extension type in uppercase hex
It's more human readable in uppercase hex string, because it's uppercase hex string in RFC.
@Piasy

This comment has been minimized.

Copy link
Contributor Author

Piasy commented Oct 10, 2018

@lminiero Updated :)

@lminiero
Copy link
Member

lminiero left a comment

Thanks for the changes! I still think this needs a couple more, though, I've added some notes inline.

Show resolved Hide resolved postprocessing/janus-pp-rec.c Outdated
Show resolved Hide resolved postprocessing/janus-pp-rec.c Outdated
@Piasy

This comment has been minimized.

Copy link
Contributor Author

Piasy commented Oct 12, 2018

Thanks for your comments! I'll update it soon.

Piasy added some commits Oct 12, 2018

read RTP padding len into another buffer
After we point `rtp` into `prebuffer`, we shouldn't change it's content if we still need access to `rtp`, otherwise, its field will be broken.
@Piasy

This comment has been minimized.

Copy link
Contributor Author

Piasy commented Oct 12, 2018

@lminiero Updated.

After we point rtp into prebuffer, we shouldn't change its content if we still need access to rtp, otherwise, its field will be broken.

So I use another prebuffer2.

@lminiero

This comment has been minimized.

Copy link
Member

lminiero commented Oct 15, 2018

@Piasy but the extension is still parsed where it was before, not when going through the video... this makes the method signature in the header useless. It should be in the *_preprocess method of the individual video processors. Besides, your RTP extension code seems to assume it will always be the first extension in the list, which might not be the case: the code we have to look for extensions in Janus is better suited for this.

Rather than ask for another change, I'll try to prepare a small branch that looks for the CVO using the Janus-based code, and integrate it in webm and mp4 processors, so that you can test it and check if it works for your case. I'll keep you posted.

@lminiero

This comment has been minimized.

Copy link
Member

lminiero commented Oct 15, 2018

@Piasy as a side note, do you have an .mjr with video orientation extension in it I can use for testing?

@Piasy

This comment has been minimized.

Copy link
Contributor Author

Piasy commented Oct 15, 2018

@lminiero I didn't understand you want to move the janus_pp_parse_rtp_header_ext_cvo calls to *_preprocess functions.

My code parse all RTP extensions, there is a while clause.

I have such mjr files, I'll attach it here.

Archive.zip

It contains 3 mjr recorded from native Android WebRTC client, native iOS WebRTC client, and browser Android client, which has 270, 90, 0 clockwise rotation.

@Piasy

This comment has been minimized.

Copy link
Contributor Author

Piasy commented Oct 15, 2018

@lminiero But why can't we parse RTP header extension at janus-pp-rec.c? My revised code doesn't need a separate loop, and we also don't need add code twice for h264/webm.

@lminiero

This comment has been minimized.

Copy link
Member

lminiero commented Oct 16, 2018

@Piasy it's ok to parse there, but it should be available to the individual processors. For instance, this is what a --parse flag on one of your recordings returns in my patch:

This is a video recording:
  -- Codec:   h264
  -- Created: 1539047403915266
  -- Written: 1539047404340191
SSRC detected: 484166380
Counted 716 RTP packets
Counted 716 frame packets
The video is rotated
Video rotation: 270 degrees
  -- 640x480 (fps [2970,3060] ~ 30)
Parsing and reordering completed, bye!

In the main code, I just count how many times the video rotated: in this case, there was just a single rotation (270), so even if the extension was there more than once, I summarize and say "The video is rotated" to clarify it's not a 0 degrees video. I take note of this rotation, and print it again in the h264 portion of the code.

As an example of other extensions, I also added support for the audio level extension, which I integrated in the audio processors. In both cases, there's not much we can do with the values we extract, but they're useful to print anyway.

I'll cleanup the code and publish it as a PR shortly, so that you can check if it works fine for your use case.

@lminiero

This comment has been minimized.

Copy link
Member

lminiero commented Oct 16, 2018

I just published the code: looking forward to your feedback!

@lminiero

This comment has been minimized.

Copy link
Member

lminiero commented Oct 16, 2018

Closing as I merged the other PR 👍

@lminiero lminiero closed this Oct 16, 2018

@Piasy Piasy deleted the Piasy:print_video_rotation branch Oct 16, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.