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

Split Video Dumps on Resolution Change #3930

Merged
merged 2 commits into from
Jun 28, 2016

Conversation

RisingFog
Copy link
Member

@RisingFog RisingFog commented Jun 25, 2016

Also handles widescreen changes.


This change is Reviewable

@RisingFog RisingFog force-pushed the split_video_dump_resolution branch from 671a194 to a7bc839 Compare June 25, 2016 02:44
@lioncash
Copy link
Member

Review status: 0 of 6 files reviewed at latest revision, 2 unresolved discussions.


Source/Core/VideoCommon/AVIDump.cpp, line 48 [r1] (raw file):

static bool s_start_dumping = false;
static u64 s_last_pts;
static int current_width;

s_


Source/Core/VideoCommon/AVIDump.cpp, line 171 [r2] (raw file):

{
  if (width != current_width || height != current_height)
  {

This should go in its own function. This doesn't have anything to do with directly adding a new frame (particularly considering it stops and then starts a new video).


Comments from Reviewable

@RisingFog RisingFog force-pushed the split_video_dump_resolution branch from a7bc839 to d5f22fa Compare June 25, 2016 02:51
@RisingFog
Copy link
Member Author

Review status: 0 of 6 files reviewed at latest revision, 2 unresolved discussions.


Source/Core/VideoCommon/AVIDump.cpp, line 48 [r1] (raw file):

Previously, lioncash (Mat M.) wrote…

s_

Done.

Source/Core/VideoCommon/AVIDump.cpp, line 171 [r2] (raw file):

Previously, lioncash (Mat M.) wrote…

This should go in its own function. This doesn't have anything to do with directly adding a new frame (particularly considering it stops and then starts a new video).

Done.

Comments from Reviewable

@RisingFog RisingFog force-pushed the split_video_dump_resolution branch 2 times, most recently from c5c8637 to 3e44fe9 Compare June 25, 2016 02:55
{
if (width != s_current_width || height != s_current_height)
{
Stop(true);

This comment was marked as off-topic.

@RisingFog RisingFog force-pushed the split_video_dump_resolution branch 3 times, most recently from 2d42916 to 9f74b40 Compare June 25, 2016 15:37
@RisingFog RisingFog force-pushed the split_video_dump_resolution branch from 9f74b40 to 88dbaf1 Compare June 25, 2016 15:39
@RisingFog
Copy link
Member Author

Review status: 0 of 2 files reviewed at latest revision, 3 unresolved discussions.


Source/Core/VideoCommon/AVIDump.cpp, line 295 [r3] (raw file):

Previously, BhaaLseN (BhaaL) wrote…

Since this is only called here with true as parameter, would it make sense to move the s_file_index change over here?

Done.

Comments from Reviewable

@RisingFog
Copy link
Member Author

@dolphin-emu-bot rebuild

@JMC47
Copy link
Contributor

JMC47 commented Jun 27, 2016

Works correctly in D3D11/12 and OpenGL.

@Helios747
Copy link
Contributor

Reviewed 2 of 6 files at r4, 2 of 2 files at r5.
Review status: all files reviewed at latest revision, 2 unresolved discussions.


Comments from Reviewable

@@ -894,11 +893,11 @@ void Renderer::SwapImpl(u32 xfb_addr, u32 fb_width, u32 fb_stride, u32 fb_height
}
if (bAVIDumping)
{
if (frame_data.empty() || w != s_record_width || h != s_record_height)
if (frame_data.empty() || source_width != s_record_width || source_height != source_height)

This comment was marked as off-topic.

@phire
Copy link
Member

phire commented Jun 27, 2016

Reviewed 2 of 6 files at r1, 2 of 6 files at r4, 2 of 2 files at r5.
Review status: all files reviewed at latest revision, 5 unresolved discussions.


Source/Core/VideoBackends/D3D/Render.cpp, line 920 [r5] (raw file):

      D3D::context->Map(s_screenshot_texture, 0, D3D11_MAP_READ, 0, &map);

      if (frame_data.empty() || source_width != s_recordWidth || source_height != s_recordHeight)

You could simplify this all to just:

if (frame_data.capacity() != 3 * source_width * source_height)
    frame_data.resize(3 * source_width * source_height)

Or swap the != for a < to only resize when frame_data is too small.

Actually, s_recordWidth/Height isn't needed at all anymore, as it's always updated to match source_width and source_height. Delete them.


Comments from Reviewable

@RisingFog RisingFog force-pushed the split_video_dump_resolution branch from 1c77f31 to 8a1bb66 Compare June 27, 2016 14:03
@RisingFog RisingFog force-pushed the split_video_dump_resolution branch from 8a1bb66 to f31adf9 Compare June 27, 2016 14:13
@RisingFog
Copy link
Member Author

Review status: 2 of 4 files reviewed at latest revision, 5 unresolved discussions.


Source/Core/VideoBackends/D3D/Render.cpp, line 920 [r5] (raw file):

Previously, phire (Scott Mansell) wrote…

You could simplify this all to just:

if (frame_data.capacity() != 3 * source_width * source_height)
    frame_data.resize(3 * source_width * source_height)

Or swap the != for a < to only resize when frame_data is too small.

Actually, s_recordWidth/Height isn't needed at all anymore, as it's always updated to match source_width and source_height. Delete them.

Done.

Source/Core/VideoBackends/D3D12/Render.cpp, line 896 [r5] (raw file):

Previously, sepalani wrote…

source_height != source_height? Not sure it was intended...

Done.

Source/Core/VideoBackends/D3D12/Render.cpp, line 899 [r5] (raw file):

Previously, sepalani wrote…

Ditto.

Done.

Comments from Reviewable

@phire
Copy link
Member

phire commented Jun 28, 2016

:lgtm:


Reviewed 2 of 2 files at r6.
Review status: all files reviewed at latest revision, 4 unresolved discussions.


Comments from Reviewable

@RisingFog RisingFog merged commit 28a3691 into dolphin-emu:master Jun 28, 2016
@RisingFog RisingFog deleted the split_video_dump_resolution branch June 28, 2016 02:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
7 participants